Return-Path: Received: from daytona.panasas.com ([67.152.220.89]:34619 "EHLO daytona.panasas.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757470Ab0LBOLH (ORCPT ); Thu, 2 Dec 2010 09:11:07 -0500 Message-ID: <4CF7A8F9.8010902@panasas.com> Date: Thu, 02 Dec 2010 16:11:05 +0200 From: Benny Halevy To: Jim Rees CC: linux-nfs@vger.kernel.org, peter honeyman Subject: Re: [PATCH 4/5] various minor cleanups References: <55dae19c431eeee6d4fabf18550fdf976646a9a5.1291142529.git.rees@umich.edu> <4CF7A64D.8040802@panasas.com> In-Reply-To: <4CF7A64D.8040802@panasas.com> Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On 2010-12-02 15:59, Benny Halevy wrote: > On 2010-11-30 21:14, Jim Rees wrote: >> Signed-off-by: Jim Rees >> --- >> utils/blkmapd/device-discovery.c | 1 + >> utils/blkmapd/device-discovery.h | 11 +-- >> utils/blkmapd/device-inq.c | 7 +- >> utils/blkmapd/device-process.c | 31 ++++--- >> utils/blkmapd/dm-device.c | 164 +++++++++++++++++-------------------- >> 5 files changed, 103 insertions(+), 111 deletions(-) >> >> diff --git a/utils/blkmapd/device-discovery.c b/utils/blkmapd/device-discovery.c >> index 0497a11..c2675b8 100644 >> --- a/utils/blkmapd/device-discovery.c >> +++ b/utils/blkmapd/device-discovery.c >> @@ -39,6 +39,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> diff --git a/utils/blkmapd/device-discovery.h b/utils/blkmapd/device-discovery.h >> index 8cf88b8..a0937b1 100644 >> --- a/utils/blkmapd/device-discovery.h >> +++ b/utils/blkmapd/device-discovery.h >> @@ -28,11 +28,10 @@ >> #define BL_DEVICE_DISCOVERY_H >> >> #include >> -#include >> >> enum blk_vol_type { >> BLOCK_VOLUME_SIMPLE = 0, /* maps to a single LU */ >> - BLOCK_VOLUME_SLICE = 1, /* slice of another volume */ >> + BLOCK_VOLUME_SLICE = 1, /* slice of another volume */ >> BLOCK_VOLUME_CONCAT = 2, /* concatenation of multiple volumes */ >> BLOCK_VOLUME_STRIPE = 3, /* striped across multiple volumes */ >> BLOCK_VOLUME_PSEUDO = 4, >> @@ -45,15 +44,15 @@ struct bl_volume { >> struct bl_volume **bv_vols; >> int bv_vol_n; >> union { >> - dev_t bv_dev; /* for BLOCK_VOLUME_SIMPLE(PSEUDO) */ >> + dev_t bv_dev; /* for BLOCK_VOLUME_SIMPLE(PSEUDO) */ >> off_t bv_stripe_unit; /* for BLOCK_VOLUME_STRIPE(CONCAT) */ >> off_t bv_offset; /* for BLOCK_VOLUME_SLICE */ >> } param; >> }; >> >> struct bl_sig_comp { >> - int64_t bs_offset; /* In bytes */ >> - uint32_t bs_length; /* In bytes */ >> + uint64_t bs_offset; /* In bytes */ >> + uint32_t bs_length; /* In bytes */ >> char *bs_string; >> }; >> >> @@ -107,7 +106,7 @@ struct pipefs_hdr { >> uint32_t msgid; >> uint8_t type; >> uint8_t flags; >> - uint16_t totallen; /* length of entire message, including hdr */ >> + uint16_t totallen; /* length of entire message, including hdr */ >> uint32_t status; >> }; >> >> diff --git a/utils/blkmapd/device-inq.c b/utils/blkmapd/device-inq.c >> index e1c0128..eabc70c 100644 >> --- a/utils/blkmapd/device-inq.c >> +++ b/utils/blkmapd/device-inq.c >> @@ -39,11 +39,12 @@ >> >> #include >> #include >> +#include >> #include >> +#include >> #include >> #include >> #include >> -#include >> #include >> #include >> >> @@ -174,10 +175,10 @@ extern enum bl_path_state_e bldev_read_ap_state(int fd) >> struct bl_serial *bldev_read_serial(int fd, const char *filename) >> { >> struct bl_serial *serial_out = NULL; >> - int status = 0, pos, len; >> + int status = 0; >> char *buffer; >> struct bl_dev_id *dev_root, *dev_id; >> - unsigned int current_id = 0; >> + unsigned int pos, len, current_id = 0; >> >> status = bldev_inquire_pages(fd, 0x83, &buffer); >> if (status) >> diff --git a/utils/blkmapd/device-process.c b/utils/blkmapd/device-process.c >> index 4482bd5..4ce47e1 100644 >> --- a/utils/blkmapd/device-process.c >> +++ b/utils/blkmapd/device-process.c >> @@ -33,29 +33,33 @@ >> * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. >> */ >> >> -#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> #include >> #include >> #include >> #include >> -#include >> -#include >> -#include >> +#include >> #include >> #include >> -#include >> -#include >> + >> #include "device-discovery.h" >> >> -static char *pretty_sig(char *sig, int siglen) >> +static char *pretty_sig(char *sig, uint32_t siglen) >> { >> static char rs[100]; >> - unsigned int i; >> + unsigned long i; >> >> - if (siglen <= 4) { >> + if (siglen <= sizeof i) { >> memcpy(&i, sig, sizeof i); >> - sprintf(rs, "0x%0x", i); >> + sprintf(rs, "0x%0lx", i); > > What about machine endianess? > The MDS and clients may be of different gender, no? > Also, on 64 bit machines, you may copy 8 bytes while the signature > is 4-bytes long so you may copy junk into i. > > Benny > >> } else { >> + if (siglen > sizeof rs - 1) >> + siglen = sizeof rs - 1; Hmm, for courtesy purposes, how about ending the truncated signature with "..."? Benny >> memcpy(rs, sig, siglen); >> rs[siglen] = '\0'; >> } >> @@ -65,6 +69,7 @@ static char *pretty_sig(char *sig, int siglen) >> uint32_t *blk_overflow(uint32_t * p, uint32_t * end, size_t nbytes) >> { >> uint32_t *q = p + ((nbytes + 3) >> 2); >> + >> if (q > end || q < p) >> return NULL; >> return p; >> @@ -73,8 +78,8 @@ uint32_t *blk_overflow(uint32_t * p, uint32_t * end, size_t nbytes) >> static int decode_blk_signature(uint32_t ** pp, uint32_t * end, >> struct bl_sig *sig) >> { >> - int i, siglen; >> - uint32_t *p = *pp; >> + int i; >> + uint32_t siglen, *p = *pp; >> >> BLK_READBUF(p, end, 4); >> READ32(sig->si_num_comps); >> @@ -288,7 +293,7 @@ static int decode_blk_volume(uint32_t **pp, uint32_t *end, >> off_t chunksize = vol->param.bv_stripe_unit; >> if ((chunksize == 0) || >> ((chunksize & (chunksize - 1)) != 0) || >> - (chunksize < (PAGE_SIZE >> 9))) >> + (chunksize < (off_t)(PAGE_SIZE >> 9))) >> return -EIO; >> BLK_READBUF(p, end, 4); >> READ32(vol->bv_vol_n); >> diff --git a/utils/blkmapd/dm-device.c b/utils/blkmapd/dm-device.c >> index 14ad131..5a9f5ec 100644 >> --- a/utils/blkmapd/dm-device.c >> +++ b/utils/blkmapd/dm-device.c >> @@ -24,15 +24,19 @@ >> * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF >> * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. >> */ >> -#include >> + >> +#include >> +#include >> +#include >> + >> #include >> #include >> #include >> -#include >> -#include >> +#include >> #include >> #include >> -#include >> +#include >> + >> #include "device-discovery.h" >> >> #define DM_DEV_NAME_LEN 256 >> @@ -66,9 +70,10 @@ static inline struct bl_dm_table *bl_dm_table_alloc(void) >> return (struct bl_dm_table *)calloc(1, sizeof(struct bl_dm_table)); >> } >> >> -void bl_dm_table_free(struct bl_dm_table *bl_table_head) >> +static void bl_dm_table_free(struct bl_dm_table *bl_table_head) >> { >> - struct bl_dm_table *p = bl_table_head; >> + struct bl_dm_table *p; >> + >> while (bl_table_head) { >> p = bl_table_head->next; >> free(bl_table_head); >> @@ -76,41 +81,39 @@ void bl_dm_table_free(struct bl_dm_table *bl_table_head) >> } >> } >> >> -void add_to_bl_dm_table(struct bl_dm_table **bl_table_head, >> +static void add_to_bl_dm_table(struct bl_dm_table **bl_table_head, >> struct bl_dm_table *table) >> { >> - struct bl_dm_table *pre; >> + struct bl_dm_table *p; >> + >> if (!*bl_table_head) { >> *bl_table_head = table; >> return; >> } >> - pre = *bl_table_head; >> - while (pre->next) >> - pre = pre->next; >> - pre->next = table; >> - return; >> + p = *bl_table_head; >> + while (p->next) >> + p = p->next; >> + p->next = table; >> } >> >> struct bl_dm_tree *bl_tree_head; >> >> -struct bl_dm_tree *find_bl_dm_tree(uint64_t dev) >> +static struct bl_dm_tree *find_bl_dm_tree(uint64_t dev) >> { >> - struct bl_dm_tree *p = bl_tree_head; >> - while (p) { >> + struct bl_dm_tree *p; >> + >> + for (p = bl_tree_head; p; p = p->next) { >> if (p->dev == dev) >> - return p; >> - p = p->next; >> + break; >> } >> - return NULL; >> + return p; >> } >> >> -void del_from_bl_dm_tree(uint64_t dev) >> +static void del_from_bl_dm_tree(uint64_t dev) >> { >> - struct bl_dm_tree *pre = bl_tree_head; >> - struct bl_dm_tree *p; >> + struct bl_dm_tree *p, *pre = bl_tree_head; >> >> - p = pre; >> - while (p) { >> + for (p = pre; p; p = p->next) { >> if (p->dev == dev) { >> pre->next = p->next; >> if (p == bl_tree_head) >> @@ -119,29 +122,31 @@ void del_from_bl_dm_tree(uint64_t dev) >> break; >> } >> pre = p; >> - p = pre->next; >> } >> } >> >> -void add_to_bl_dm_tree(struct bl_dm_tree *tree) >> +static void add_to_bl_dm_tree(struct bl_dm_tree *tree) >> { >> - struct bl_dm_tree *pre; >> + struct bl_dm_tree *p; >> + >> if (!bl_tree_head) { >> bl_tree_head = tree; >> return; >> } >> - pre = bl_tree_head; >> - while (pre->next) >> - pre = pre->next; >> - pre->next = tree; >> + p = bl_tree_head; >> + while (p->next) >> + p = p->next; >> + p->next = tree; >> return; >> } >> >> -/* Create device via device mapper >> +/* >> + * Create device via device mapper >> * return 0 when creation failed >> * return dev no for created device >> */ >> -uint64_t dm_single_device_create(const char *dev_name, struct bl_dm_table * p) >> +static uint64_t >> +dm_single_device_create(const char *dev_name, struct bl_dm_table * p) >> { >> struct dm_task *dmt; >> struct dm_info dminfo; >> @@ -182,21 +187,19 @@ uint64_t dm_single_device_create(const char *dev_name, struct bl_dm_table * p) >> return MKDEV(dminfo.major, dminfo.minor); >> } >> >> -int dm_device_remove_byname(const char *dev_name) >> +static int dm_device_remove_byname(const char *dev_name) >> { >> struct dm_task *dmt; >> int ret = 0; >> >> dmt = dm_task_create(DM_DEVICE_REMOVE); >> if (!dmt) >> - return -ENODEV; >> + return 0; >> >> ret = dm_task_set_name(dmt, dev_name) && dm_task_run(dmt); >> >> dm_task_update_nodes(); >> - >> - if (dmt) >> - dm_task_destroy(dmt); >> + dm_task_destroy(dmt); >> >> return ret; >> } >> @@ -205,61 +208,64 @@ int dm_device_remove(uint64_t dev) >> { >> struct dm_task *dmt; >> struct dm_names *dmnames; >> - char *names = NULL; >> - int ret = -1; >> + char *name = NULL; >> + int ret = 0; >> >> /* Look for dev_name via dev, if dev_name could be transferred here, >> we could jump to DM_DEVICE_REMOVE directly */ >> + >> dmt = dm_task_create(DM_DEVICE_LIST); >> if (!dmt) { >> BL_LOG_ERR("dm_task creation failed\n"); >> - return -ENODEV; >> + goto out; >> } >> >> ret = dm_task_run(dmt); >> if (!ret) { >> BL_LOG_ERR("dm_task_run failed\n"); >> - goto error; >> + goto out; >> } >> >> dmnames = dm_task_get_names(dmt); >> if (!dmnames || !dmnames->dev) { >> BL_LOG_ERR("dm_task_get_names failed\n"); >> - goto error; >> + goto out; >> } >> >> - do { >> + while (dmnames) { >> if (dmnames->dev == dev) { >> - names = dmnames->name; >> + name = dmnames->name; >> break; >> } >> dmnames = (void *)dmnames + dmnames->next; >> - } while (dmnames); >> + } >> >> - if (!names) { >> + if (!name) { >> BL_LOG_ERR("Could not find device\n"); >> - goto error; >> + goto out; >> } >> >> dm_task_update_nodes(); >> >> - error: >> - dm_task_destroy(dmt); >> + out: >> + if (dmt) >> + dm_task_destroy(dmt); >> >> /* Start to remove device */ >> - if (names) >> - ret = dm_device_remove_byname(names); >> + if (name) >> + ret = dm_device_remove_byname(name); >> + >> return ret; >> } >> >> static unsigned long dev_count; >> >> -void dm_devicelist_remove(unsigned long start, unsigned long end) >> +static void dm_devicelist_remove(unsigned long start, unsigned long end) >> { >> char dev_name[DM_DEV_NAME_LEN]; >> unsigned long count; >> >> - if ((start >= dev_count) || (end <= 1) || (start >= end - 1)) >> + if (start >= dev_count || end <= 1 || start >= end - 1) >> return; >> >> for (count = end - 1; count > start; count--) { >> @@ -270,7 +276,7 @@ void dm_devicelist_remove(unsigned long start, unsigned long end) >> return; >> } >> >> -void bl_dm_remove_tree(uint64_t dev) >> +static void bl_dm_remove_tree(uint64_t dev) >> { >> struct bl_dm_tree *p; >> >> @@ -282,28 +288,28 @@ void bl_dm_remove_tree(uint64_t dev) >> del_from_bl_dm_tree(dev); >> } >> >> -void bl_dm_create_tree(uint64_t dev) >> +static int bl_dm_create_tree(uint64_t dev) >> { >> struct dm_tree *tree; >> struct bl_dm_tree *bl_tree; >> >> bl_tree = find_bl_dm_tree(dev); >> if (bl_tree) >> - return; /* XXX: error? */ >> + return 1; >> >> tree = dm_tree_create(); >> if (!tree) >> - return; >> + return 0; >> >> if (!dm_tree_add_dev(tree, MAJOR(dev), MINOR(dev))) { >> dm_tree_free(tree); >> - return; >> + return 0; >> } >> >> bl_tree = malloc(sizeof(struct bl_dm_tree)); >> if (!bl_tree) { >> dm_tree_free(tree); >> - return; >> + return 0; >> } >> >> bl_tree->dev = dev; >> @@ -311,29 +317,7 @@ void bl_dm_create_tree(uint64_t dev) >> bl_tree->next = NULL; >> add_to_bl_dm_tree(bl_tree); >> >> - return; >> -} >> - >> -uint64_t dm_device_nametodev(char *dev_name) >> -{ >> - struct dm_task *dmt; >> - int ret = 0; >> - struct dm_info dminfo; >> - >> - dmt = dm_task_create(DM_DEVICE_INFO); >> - if (!dmt) >> - return -ENODEV; >> - >> - ret = dm_task_set_name(dmt, dev_name) && >> - dm_task_run(dmt) && dm_task_get_info(dmt, &dminfo); >> - >> - if (dmt) >> - dm_task_destroy(dmt); >> - >> - if (!ret) >> - return 0; >> - >> - return MKDEV(dminfo.major, dminfo.minor); >> + return 1; >> } >> >> int dm_device_remove_all(uint64_t *dev) >> @@ -364,6 +348,7 @@ int dm_device_remove_all(uint64_t *dev) >> ret = dm_tree_deactivate_children(node, uuid, strlen(uuid)); >> dm_task_update_nodes(); >> bl_dm_remove_tree(bl_dev); >> + >> return ret; >> } >> >> @@ -372,7 +357,7 @@ uint64_t dm_device_create(struct bl_volume *vols, int num_vols) >> { >> uint64_t size, dev = 0; >> unsigned long count = dev_count; >> - int number = 0, i, pos; >> + int volnum, i, pos; >> struct bl_volume *node; >> char *tmp; >> struct bl_dm_table *table = NULL; >> @@ -381,8 +366,8 @@ uint64_t dm_device_create(struct bl_volume *vols, int num_vols) >> char *dev_name = NULL; >> >> /* Create pseudo device here */ >> - while (number < num_vols) { >> - node = &vols[number]; >> + for (volnum = 0; volnum < num_vols; volnum++) { >> + node = &vols[volnum]; >> switch (node->bv_type) { >> case BLOCK_VOLUME_SIMPLE: >> /* Do not need to create device here */ >> @@ -492,16 +477,17 @@ uint64_t dm_device_create(struct bl_volume *vols, int num_vols) >> node->param.bv_dev = dev; >> /* TODO: extend use with PSEUDO later */ >> node->bv_type = BLOCK_VOLUME_PSEUDO; >> + >> continued: >> - number++; >> if (bl_table_head) >> bl_dm_table_free(bl_table_head); >> bl_table_head = NULL; >> } >> out: >> - if (bl_table_head) >> + if (bl_table_head) { >> bl_dm_table_free(bl_table_head); >> - bl_table_head = NULL; >> + bl_table_head = NULL; >> + } >> if (dev) >> bl_dm_create_tree(dev); >> if (dev_name) > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html