| Message ID | 504A09BA.5030203@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/07/2012 07:50 AM, Alex Elder wrote: > An rbd_dev structure maintains a list of current snapshots that have > already been fully initialized. The entries on the list have type > struct rbd_snap, and each entry contains a copy of information > that's found in the rbd_dev's snapshot context and header. > > The only caller of snap_by_name() is rbd_header_set_snap(). In that > call site any positive return value (the index in the snapshot > array) is ignored. > > rbd_header_set_snap() also has only one caller--rbd_add()--and that > call is made after a call to rbd_dev_snap_devs_update(). Because > the rbd_snap structures are initialized in that function, the > current snapshot list can be used instead of the snapshot context to > look up a snapshot's information by name. > > Change snap_by_name() so it uses the snapshot list rather than the > rbd_dev's snapshot context in looking up snapshot information. > Return 0 if it's found rather than the snapshot id. To do this, > change rbd_snap_by_name() to take an rbd_dev rather than > rbd_image_header structure pointer as its first argument. This argument change was an earlier commit now. > No caller ever passes a null pointer to snap_by_name() for the > snapshot id or the size, so just assign using those pointers > unconditionally. No caller passes id or size now either. With the commit message fixed: Reviewed-by: Josh Durgin <josh.durgin@inktank.com> > Signed-off-by: Alex Elder <elder@inktank.com> > --- > drivers/block/rbd.c | 19 +++++++------------ > 1 file changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index e922989..4dff92f 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -623,23 +623,18 @@ out_err: > > static int snap_by_name(struct rbd_device *rbd_dev, const char *snap_name) > { > - int i; > - struct rbd_image_header *header = &rbd_dev->header; > - char *p = header->snap_names; > - > - rbd_assert(header->snapc != NULL); > - for (i = 0; i < header->snapc->num_snaps; i++) { > - if (!strcmp(snap_name, p)) { > > - /* Found it. Pass back its id and/or size */ > + struct rbd_snap *snap; > > - rbd_dev->mapping.snap_id = header->snapc->snaps[i]; > - rbd_dev->mapping.size = header->snap_sizes[i]; > + list_for_each_entry(snap, &rbd_dev->snaps, node) { > + if (!strcmp(snap_name, snap->name)) { > + rbd_dev->mapping.snap_id = snap->id; > + rbd_dev->mapping.size = snap->size; > > - return i; > + return 0; > } > - p += strlen(p) + 1; /* Skip ahead to the next name */ > } > + > return -ENOENT; > } > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/10/2012 05:32 PM, Josh Durgin wrote: > On 09/07/2012 07:50 AM, Alex Elder wrote: >> An rbd_dev structure maintains a list of current snapshots that have >> already been fully initialized. The entries on the list have type >> struct rbd_snap, and each entry contains a copy of information >> that's found in the rbd_dev's snapshot context and header. >> >> The only caller of snap_by_name() is rbd_header_set_snap(). In that >> call site any positive return value (the index in the snapshot >> array) is ignored. >> >> rbd_header_set_snap() also has only one caller--rbd_add()--and that >> call is made after a call to rbd_dev_snap_devs_update(). Because >> the rbd_snap structures are initialized in that function, the >> current snapshot list can be used instead of the snapshot context to >> look up a snapshot's information by name. >> >> Change snap_by_name() so it uses the snapshot list rather than the >> rbd_dev's snapshot context in looking up snapshot information. >> Return 0 if it's found rather than the snapshot id. To do this, >> change rbd_snap_by_name() to take an rbd_dev rather than >> rbd_image_header structure pointer as its first argument. > > This argument change was an earlier commit now. Whoops. I need to pay more attention to the descriptions before I post for review I guess. Apparently I decided to pull that change into its own patch somewhere along the way... >> No caller ever passes a null pointer to snap_by_name() for the >> snapshot id or the size, so just assign using those pointers >> unconditionally. > > No caller passes id or size now either. Here again, moved that particular change elsewhere. Good eye. > With the commit message fixed: Will do. > Reviewed-by: Josh Durgin <josh.durgin@inktank.com> > >> Signed-off-by: Alex Elder <elder@inktank.com> >> --- >> drivers/block/rbd.c | 19 +++++++------------ >> 1 file changed, 7 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >> index e922989..4dff92f 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -623,23 +623,18 @@ out_err: >> >> static int snap_by_name(struct rbd_device *rbd_dev, const char >> *snap_name) >> { >> - int i; >> - struct rbd_image_header *header = &rbd_dev->header; >> - char *p = header->snap_names; >> - >> - rbd_assert(header->snapc != NULL); >> - for (i = 0; i < header->snapc->num_snaps; i++) { >> - if (!strcmp(snap_name, p)) { >> >> - /* Found it. Pass back its id and/or size */ >> + struct rbd_snap *snap; >> >> - rbd_dev->mapping.snap_id = header->snapc->snaps[i]; >> - rbd_dev->mapping.size = header->snap_sizes[i]; >> + list_for_each_entry(snap, &rbd_dev->snaps, node) { >> + if (!strcmp(snap_name, snap->name)) { >> + rbd_dev->mapping.snap_id = snap->id; >> + rbd_dev->mapping.size = snap->size; >> >> - return i; >> + return 0; >> } >> - p += strlen(p) + 1; /* Skip ahead to the next name */ >> } >> + >> return -ENOENT; >> } >> > > > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index e922989..4dff92f 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -623,23 +623,18 @@ out_err: static int snap_by_name(struct rbd_device *rbd_dev, const char *snap_name) { - int i; - struct rbd_image_header *header = &rbd_dev->header; - char *p = header->snap_names; - - rbd_assert(header->snapc != NULL); - for (i = 0; i < header->snapc->num_snaps; i++) { - if (!strcmp(snap_name, p)) { - /* Found it. Pass back its id and/or size */ + struct rbd_snap *snap; - rbd_dev->mapping.snap_id = header->snapc->snaps[i]; - rbd_dev->mapping.size = header->snap_sizes[i]; + list_for_each_entry(snap, &rbd_dev->snaps, node) { + if (!strcmp(snap_name, snap->name)) { + rbd_dev->mapping.snap_id = snap->id; + rbd_dev->mapping.size = snap->size; - return i; + return 0; } - p += strlen(p) + 1; /* Skip ahead to the next name */ } + return -ENOENT; }
An rbd_dev structure maintains a list of current snapshots that have already been fully initialized. The entries on the list have type struct rbd_snap, and each entry contains a copy of information that's found in the rbd_dev's snapshot context and header. The only caller of snap_by_name() is rbd_header_set_snap(). In that call site any positive return value (the index in the snapshot array) is ignored. rbd_header_set_snap() also has only one caller--rbd_add()--and that call is made after a call to rbd_dev_snap_devs_update(). Because the rbd_snap structures are initialized in that function, the current snapshot list can be used instead of the snapshot context to look up a snapshot's information by name. Change snap_by_name() so it uses the snapshot list rather than the rbd_dev's snapshot context in looking up snapshot information. Return 0 if it's found rather than the snapshot id. To do this, change rbd_snap_by_name() to take an rbd_dev rather than rbd_image_header structure pointer as its first argument. No caller ever passes a null pointer to snap_by_name() for the snapshot id or the size, so just assign using those pointers unconditionally. Signed-off-by: Alex Elder <elder@inktank.com> --- drivers/block/rbd.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)