| Message ID | 1406191369-6746-2-git-send-email-ilya.dryomov@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/24/2014 03:42 AM, Ilya Dryomov wrote: > Make /sys/bus/rbd/devices/<id>/parent show the entire chain of parent > images. While at it, kernel sprintf() doesn't return negative values, > casting to unsigned long long is no longer necessary and there is no > good reason to split into multiple sprintf() calls. I like the use of a single snprintf() call, it is much less cumbersome. The reason I always cast u64 values to (unsigned long long) in printf() calls is because on some 64-bit architectures, u64 is defined as simply (unsigned long), so without the cast they spit out warnings. I hate this, but it is reality (see include/asm-generic/{int-l64.h,int-ll64.h}). You can alternatively use %PRIu64 rather than %llu in your format strings, but I think I hate that more. Anyway, if you want to avoid warnings on all architectures you should fix that. As another aside, I've been too timid to use the ?: conditional expression without its middle operand. I have no objection to it at all, I like it. I bring it up because I recently got burned for using a gcc feature that wasn't supported on older compilers (unnamed struct/union fields)--specifically a version newer than gcc 3.2, which is the minimum supported version for the kernel (see Documentation/Changes). But fear not! That extension is supported in gcc 3.2: https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/Conditionals.html#Conditionals Just FYI... Reviewed-by: Alex Elder <elder@linaro.org> > Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> > --- > Documentation/ABI/testing/sysfs-bus-rbd | 4 +-- > drivers/block/rbd.c | 56 +++++++++++++------------------ > 2 files changed, 25 insertions(+), 35 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-rbd b/Documentation/ABI/testing/sysfs-bus-rbd > index 501adc2a9ec7..2ddd680929d8 100644 > --- a/Documentation/ABI/testing/sysfs-bus-rbd > +++ b/Documentation/ABI/testing/sysfs-bus-rbd > @@ -94,5 +94,5 @@ current_snap > > parent > > - Information identifying the pool, image, and snapshot id for > - the parent image in a layered rbd image (format 2 only). > + Information identifying the chain of parent images in a layered rbd > + image. Entries are separated by empty lines. > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 703b728e05fa..7847fbb949ff 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -3685,46 +3685,36 @@ static ssize_t rbd_snap_show(struct device *dev, > } > > /* > - * For an rbd v2 image, shows the pool id, image id, and snapshot id > - * for the parent image. If there is no parent, simply shows > - * "(no parent image)". > + * For a v2 image, shows the chain of parent images, separated by empty > + * lines. For v1 images or if there is no parent, shows "(no parent > + * image)". > */ > static ssize_t rbd_parent_show(struct device *dev, > - struct device_attribute *attr, > - char *buf) > + struct device_attribute *attr, > + char *buf) > { > struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); > - struct rbd_spec *spec = rbd_dev->parent_spec; > - int count; > - char *bufp = buf; > + ssize_t count = 0; > > - if (!spec) > + if (!rbd_dev->parent) > return sprintf(buf, "(no parent image)\n"); > > - count = sprintf(bufp, "pool_id %llu\npool_name %s\n", > - (unsigned long long) spec->pool_id, spec->pool_name); > - if (count < 0) > - return count; > - bufp += count; > - > - count = sprintf(bufp, "image_id %s\nimage_name %s\n", spec->image_id, > - spec->image_name ? spec->image_name : "(unknown)"); > - if (count < 0) > - return count; > - bufp += count; > - > - count = sprintf(bufp, "snap_id %llu\nsnap_name %s\n", > - (unsigned long long) spec->snap_id, spec->snap_name); > - if (count < 0) > - return count; > - bufp += count; > - > - count = sprintf(bufp, "overlap %llu\n", rbd_dev->parent_overlap); > - if (count < 0) > - return count; > - bufp += count; > - > - return (ssize_t) (bufp - buf); > + for ( ; rbd_dev->parent; rbd_dev = rbd_dev->parent) { > + struct rbd_spec *spec = rbd_dev->parent_spec; > + > + count += sprintf(&buf[count], "%s" > + "pool_id %llu\npool_name %s\n" > + "image_id %s\nimage_name %s\n" > + "snap_id %llu\nsnap_name %s\n" > + "overlap %llu\n", > + !count ? "" : "\n", /* first? */ > + spec->pool_id, spec->pool_name, > + spec->image_id, spec->image_name ?: "(unknown)", > + spec->snap_id, spec->snap_name, > + rbd_dev->parent_overlap); > + } > + > + return count; > } > > static ssize_t rbd_image_refresh(struct device *dev, > -- 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 Thu, Jul 24, 2014 at 4:31 PM, Alex Elder <elder@ieee.org> wrote: > On 07/24/2014 03:42 AM, Ilya Dryomov wrote: >> Make /sys/bus/rbd/devices/<id>/parent show the entire chain of parent >> images. While at it, kernel sprintf() doesn't return negative values, >> casting to unsigned long long is no longer necessary and there is no >> good reason to split into multiple sprintf() calls. > > I like the use of a single snprintf() call, it is much less > cumbersome. > > The reason I always cast u64 values to (unsigned long long) > in printf() calls is because on some 64-bit architectures, > u64 is defined as simply (unsigned long), so without the > cast they spit out warnings. I hate this, but it is reality > (see include/asm-generic/{int-l64.h,int-ll64.h}). You can > alternatively use %PRIu64 rather than %llu in your format > strings, but I think I hate that more. Anyway, if you want > to avoid warnings on all architectures you should fix that. IIRC all architectures nowadays use int-ll64.h. int-l64.h is used only in userspace for compatibility. Thanks, Ilya -- 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/Documentation/ABI/testing/sysfs-bus-rbd b/Documentation/ABI/testing/sysfs-bus-rbd index 501adc2a9ec7..2ddd680929d8 100644 --- a/Documentation/ABI/testing/sysfs-bus-rbd +++ b/Documentation/ABI/testing/sysfs-bus-rbd @@ -94,5 +94,5 @@ current_snap parent - Information identifying the pool, image, and snapshot id for - the parent image in a layered rbd image (format 2 only). + Information identifying the chain of parent images in a layered rbd + image. Entries are separated by empty lines. diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 703b728e05fa..7847fbb949ff 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -3685,46 +3685,36 @@ static ssize_t rbd_snap_show(struct device *dev, } /* - * For an rbd v2 image, shows the pool id, image id, and snapshot id - * for the parent image. If there is no parent, simply shows - * "(no parent image)". + * For a v2 image, shows the chain of parent images, separated by empty + * lines. For v1 images or if there is no parent, shows "(no parent + * image)". */ static ssize_t rbd_parent_show(struct device *dev, - struct device_attribute *attr, - char *buf) + struct device_attribute *attr, + char *buf) { struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); - struct rbd_spec *spec = rbd_dev->parent_spec; - int count; - char *bufp = buf; + ssize_t count = 0; - if (!spec) + if (!rbd_dev->parent) return sprintf(buf, "(no parent image)\n"); - count = sprintf(bufp, "pool_id %llu\npool_name %s\n", - (unsigned long long) spec->pool_id, spec->pool_name); - if (count < 0) - return count; - bufp += count; - - count = sprintf(bufp, "image_id %s\nimage_name %s\n", spec->image_id, - spec->image_name ? spec->image_name : "(unknown)"); - if (count < 0) - return count; - bufp += count; - - count = sprintf(bufp, "snap_id %llu\nsnap_name %s\n", - (unsigned long long) spec->snap_id, spec->snap_name); - if (count < 0) - return count; - bufp += count; - - count = sprintf(bufp, "overlap %llu\n", rbd_dev->parent_overlap); - if (count < 0) - return count; - bufp += count; - - return (ssize_t) (bufp - buf); + for ( ; rbd_dev->parent; rbd_dev = rbd_dev->parent) { + struct rbd_spec *spec = rbd_dev->parent_spec; + + count += sprintf(&buf[count], "%s" + "pool_id %llu\npool_name %s\n" + "image_id %s\nimage_name %s\n" + "snap_id %llu\nsnap_name %s\n" + "overlap %llu\n", + !count ? "" : "\n", /* first? */ + spec->pool_id, spec->pool_name, + spec->image_id, spec->image_name ?: "(unknown)", + spec->snap_id, spec->snap_name, + rbd_dev->parent_overlap); + } + + return count; } static ssize_t rbd_image_refresh(struct device *dev,
Make /sys/bus/rbd/devices/<id>/parent show the entire chain of parent images. While at it, kernel sprintf() doesn't return negative values, casting to unsigned long long is no longer necessary and there is no good reason to split into multiple sprintf() calls. Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> --- Documentation/ABI/testing/sysfs-bus-rbd | 4 +-- drivers/block/rbd.c | 56 +++++++++++++------------------ 2 files changed, 25 insertions(+), 35 deletions(-)