| Message ID | 1304799357-19281-1-git-send-email-weil@mail.berlios.de |
|---|---|
| State | Superseded |
| Headers | show |
Am 07.05.2011 22:15, schrieb Stefan Weil: > cppcheck report: > rbd.c:246: style: Variable 'snap' is assigned a value that is never used > > Remove snap and the related code. > > Cc: Christian Brunner<chb@muc.de> > Cc: Kevin Wolf<kwolf@redhat.com> > Signed-off-by: Stefan Weil<weil@mail.berlios.de> > --- > block/rbd.c | 4 ---- > 1 files changed, 0 insertions(+), 4 deletions(-) > > diff --git a/block/rbd.c b/block/rbd.c > index 249a590..5c7d44e 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -524,7 +524,6 @@ static int rbd_open(BlockDriverState *bs, const char *filename, int flags) > RbdHeader1 *header; > char pool[RBD_MAX_SEG_NAME_SIZE]; > char snap_buf[RBD_MAX_SEG_NAME_SIZE]; > - char *snap = NULL; > char *hbuf = NULL; > int r; > > @@ -533,9 +532,6 @@ static int rbd_open(BlockDriverState *bs, const char *filename, int flags) > s->name, sizeof(s->name))< 0) { > return -EINVAL; > } > - if (snap_buf[0] != '\0') { > - snap = snap_buf; > - } > > if ((r = rados_initialize(0, NULL))< 0) { > error_report("error initializing"); > What about this patch? Can it be applied to the block branch? Regards, Stefan W.
2011/5/22 Stefan Weil <weil@mail.berlios.de>: > Am 07.05.2011 22:15, schrieb Stefan Weil: >> >> cppcheck report: >> rbd.c:246: style: Variable 'snap' is assigned a value that is never used >> >> Remove snap and the related code. >> >> Cc: Christian Brunner<chb@muc.de> >> Cc: Kevin Wolf<kwolf@redhat.com> >> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >> --- >> block/rbd.c | 4 ---- >> 1 files changed, 0 insertions(+), 4 deletions(-) >> >> diff --git a/block/rbd.c b/block/rbd.c >> index 249a590..5c7d44e 100644 >> --- a/block/rbd.c >> +++ b/block/rbd.c >> @@ -524,7 +524,6 @@ static int rbd_open(BlockDriverState *bs, const char >> *filename, int flags) >> RbdHeader1 *header; >> char pool[RBD_MAX_SEG_NAME_SIZE]; >> char snap_buf[RBD_MAX_SEG_NAME_SIZE]; >> - char *snap = NULL; >> char *hbuf = NULL; >> int r; >> >> @@ -533,9 +532,6 @@ static int rbd_open(BlockDriverState *bs, const char >> *filename, int flags) >> s->name, sizeof(s->name))< 0) { >> return -EINVAL; >> } >> - if (snap_buf[0] != '\0') { >> - snap = snap_buf; >> - } >> >> if ((r = rados_initialize(0, NULL))< 0) { >> error_report("error initializing"); >> > > What about this patch? Can it be applied to the block branch? > > Regards, > Stefan W. No objections on my side. You can add: Reviewed-by: Christian Brunner <chb@muc.de> The questions is how we continue with the rbd driver. Recent ceph versions had some changes in librados that are incompatible with the current driver. We have to options now: 1. Change the function calls for new librados versions (I could provide a patch for this). 2. Use librbd (see Josh's patches). Using librbd simplifies the qemu driver a lot and gives us consistency with the kernel driver. - I would prefer this. (Please note that there is a race condition in the current librbd versions, that crashes qemu under high i/o load, but I'm fairly confident, that Josh will have sorted this out by the time 0.15 is released). Regards, Christian
On Mon, May 23, 2011 at 10:01 AM, Christian Brunner <chb@muc.de> wrote: > 2011/5/22 Stefan Weil <weil@mail.berlios.de>: >> Am 07.05.2011 22:15, schrieb Stefan Weil: >>> >>> cppcheck report: >>> rbd.c:246: style: Variable 'snap' is assigned a value that is never used >>> >>> Remove snap and the related code. >>> >>> Cc: Christian Brunner<chb@muc.de> >>> Cc: Kevin Wolf<kwolf@redhat.com> >>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>> --- >>> block/rbd.c | 4 ---- >>> 1 files changed, 0 insertions(+), 4 deletions(-) >>> >>> diff --git a/block/rbd.c b/block/rbd.c >>> index 249a590..5c7d44e 100644 >>> --- a/block/rbd.c >>> +++ b/block/rbd.c >>> @@ -524,7 +524,6 @@ static int rbd_open(BlockDriverState *bs, const char >>> *filename, int flags) >>> RbdHeader1 *header; >>> char pool[RBD_MAX_SEG_NAME_SIZE]; >>> char snap_buf[RBD_MAX_SEG_NAME_SIZE]; >>> - char *snap = NULL; >>> char *hbuf = NULL; >>> int r; >>> >>> @@ -533,9 +532,6 @@ static int rbd_open(BlockDriverState *bs, const char >>> *filename, int flags) >>> s->name, sizeof(s->name))< 0) { >>> return -EINVAL; >>> } >>> - if (snap_buf[0] != '\0') { >>> - snap = snap_buf; >>> - } >>> >>> if ((r = rados_initialize(0, NULL))< 0) { >>> error_report("error initializing"); >>> >> >> What about this patch? Can it be applied to the block branch? >> >> Regards, >> Stefan W. > > No objections on my side. You can add: > > Reviewed-by: Christian Brunner <chb@muc.de> > > The questions is how we continue with the rbd driver. Recent ceph > versions had some changes in librados that are incompatible with the > current driver. We have to options now: > > 1. Change the function calls for new librados versions (I could > provide a patch for this). > 2. Use librbd (see Josh's patches). These are the patches that Josh posted a while ago but haven't been reviewed yet? I think they just need some attention and then should be merged. Stefan
Am 23.05.2011 11:01, schrieb Christian Brunner: > 2011/5/22 Stefan Weil <weil@mail.berlios.de>: >> Am 07.05.2011 22:15, schrieb Stefan Weil: >>> >>> cppcheck report: >>> rbd.c:246: style: Variable 'snap' is assigned a value that is never used >>> >>> Remove snap and the related code. >>> >>> Cc: Christian Brunner<chb@muc.de> >>> Cc: Kevin Wolf<kwolf@redhat.com> >>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>> --- >>> block/rbd.c | 4 ---- >>> 1 files changed, 0 insertions(+), 4 deletions(-) >>> >>> diff --git a/block/rbd.c b/block/rbd.c >>> index 249a590..5c7d44e 100644 >>> --- a/block/rbd.c >>> +++ b/block/rbd.c >>> @@ -524,7 +524,6 @@ static int rbd_open(BlockDriverState *bs, const char >>> *filename, int flags) >>> RbdHeader1 *header; >>> char pool[RBD_MAX_SEG_NAME_SIZE]; >>> char snap_buf[RBD_MAX_SEG_NAME_SIZE]; >>> - char *snap = NULL; >>> char *hbuf = NULL; >>> int r; >>> >>> @@ -533,9 +532,6 @@ static int rbd_open(BlockDriverState *bs, const char >>> *filename, int flags) >>> s->name, sizeof(s->name))< 0) { >>> return -EINVAL; >>> } >>> - if (snap_buf[0] != '\0') { >>> - snap = snap_buf; >>> - } >>> >>> if ((r = rados_initialize(0, NULL))< 0) { >>> error_report("error initializing"); >>> >> >> What about this patch? Can it be applied to the block branch? >> >> Regards, >> Stefan W. > > No objections on my side. You can add: > > Reviewed-by: Christian Brunner <chb@muc.de> > > The questions is how we continue with the rbd driver. Recent ceph > versions had some changes in librados that are incompatible with the > current driver. We have to options now: > > 1. Change the function calls for new librados versions (I could > provide a patch for this). > 2. Use librbd (see Josh's patches). > > Using librbd simplifies the qemu driver a lot and gives us consistency > with the kernel driver. - I would prefer this. (Please note that there > is a race condition in the current librbd versions, that crashes qemu > under high i/o load, but I'm fairly confident, that Josh will have > sorted this out by the time 0.15 is released). The problem with Josh's patches (or basically anything related to the rbd driver) is that it hasn't received any review. I'm not familiar with librados and librbd, so reviewing rbd is even harder than other patches of the same size for me. Additionally, it's not a test environment that I have set up. So going forward with it, I think we need a separate rbd maintainer. So Christian, I think it would be helpful if you at least reviewed any rbd patch and either comment on it or send an Acked-by, which basically tells me to commit it without any further checks. Or maybe we should consider that you send pull requests yourself if the patches touch only rbd code. Kevin
Am 23.05.2011 12:26, schrieb Kevin Wolf: > Am 23.05.2011 11:01, schrieb Christian Brunner: >> 2011/5/22 Stefan Weil <weil@mail.berlios.de>: >>> Am 07.05.2011 22:15, schrieb Stefan Weil: >>>> >>>> cppcheck report: >>>> rbd.c:246: style: Variable 'snap' is assigned a value that is never >>>> used >>>> >>>> Remove snap and the related code. >>>> >>>> Cc: Christian Brunner<chb@muc.de> >>>> Cc: Kevin Wolf<kwolf@redhat.com> >>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>>> --- >>>> block/rbd.c | 4 ---- >>>> 1 files changed, 0 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/block/rbd.c b/block/rbd.c >>>> index 249a590..5c7d44e 100644 >>>> --- a/block/rbd.c >>>> +++ b/block/rbd.c >>>> @@ -524,7 +524,6 @@ static int rbd_open(BlockDriverState *bs, const >>>> char >>>> *filename, int flags) >>>> RbdHeader1 *header; >>>> char pool[RBD_MAX_SEG_NAME_SIZE]; >>>> char snap_buf[RBD_MAX_SEG_NAME_SIZE]; >>>> - char *snap = NULL; >>>> char *hbuf = NULL; >>>> int r; >>>> >>>> @@ -533,9 +532,6 @@ static int rbd_open(BlockDriverState *bs, const >>>> char >>>> *filename, int flags) >>>> s->name, sizeof(s->name))< 0) { >>>> return -EINVAL; >>>> } >>>> - if (snap_buf[0] != '\0') { >>>> - snap = snap_buf; >>>> - } >>>> >>>> if ((r = rados_initialize(0, NULL))< 0) { >>>> error_report("error initializing"); >>>> >>> >>> What about this patch? Can it be applied to the block branch? >>> >>> Regards, >>> Stefan W. >> >> No objections on my side. You can add: >> >> Reviewed-by: Christian Brunner <chb@muc.de> >> >> The questions is how we continue with the rbd driver. Recent ceph >> versions had some changes in librados that are incompatible with the >> current driver. We have to options now: >> >> 1. Change the function calls for new librados versions (I could >> provide a patch for this). >> 2. Use librbd (see Josh's patches). >> >> Using librbd simplifies the qemu driver a lot and gives us consistency >> with the kernel driver. - I would prefer this. (Please note that there >> is a race condition in the current librbd versions, that crashes qemu >> under high i/o load, but I'm fairly confident, that Josh will have >> sorted this out by the time 0.15 is released). > > The problem with Josh's patches (or basically anything related to the > rbd driver) is that it hasn't received any review. I'm not familiar with > librados and librbd, so reviewing rbd is even harder than other patches > of the same size for me. Additionally, it's not a test environment that > I have set up. > > So going forward with it, I think we need a separate rbd maintainer. So > Christian, I think it would be helpful if you at least reviewed any rbd > patch and either comment on it or send an Acked-by, which basically > tells me to commit it without any further checks. Or maybe we should > consider that you send pull requests yourself if the patches touch only > rbd code. > > Kevin This patch was reviewed by Christian, but is still in my queue of open patches. Kevin, could you please take it into the block queue? Thanks, Stefan W.
2011/5/27 Stefan Weil <weil@mail.berlios.de>: > Am 23.05.2011 12:26, schrieb Kevin Wolf: >> >> Am 23.05.2011 11:01, schrieb Christian Brunner: >>> >>> 2011/5/22 Stefan Weil <weil@mail.berlios.de>: >>>> >>>> Am 07.05.2011 22:15, schrieb Stefan Weil: >>>>> >>>>> cppcheck report: >>>>> rbd.c:246: style: Variable 'snap' is assigned a value that is never >>>>> used >>>>> >>>>> Remove snap and the related code. >>>>> >>>>> Cc: Christian Brunner<chb@muc.de> >>>>> Cc: Kevin Wolf<kwolf@redhat.com> >>>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de> >>>>> --- >>>>> block/rbd.c | 4 ---- >>>>> 1 files changed, 0 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/block/rbd.c b/block/rbd.c >>>>> index 249a590..5c7d44e 100644 >>>>> --- a/block/rbd.c >>>>> +++ b/block/rbd.c >>>>> @@ -524,7 +524,6 @@ static int rbd_open(BlockDriverState *bs, const >>>>> char >>>>> *filename, int flags) >>>>> RbdHeader1 *header; >>>>> char pool[RBD_MAX_SEG_NAME_SIZE]; >>>>> char snap_buf[RBD_MAX_SEG_NAME_SIZE]; >>>>> - char *snap = NULL; >>>>> char *hbuf = NULL; >>>>> int r; >>>>> >>>>> @@ -533,9 +532,6 @@ static int rbd_open(BlockDriverState *bs, const >>>>> char >>>>> *filename, int flags) >>>>> s->name, sizeof(s->name))< 0) { >>>>> return -EINVAL; >>>>> } >>>>> - if (snap_buf[0] != '\0') { >>>>> - snap = snap_buf; >>>>> - } >>>>> >>>>> if ((r = rados_initialize(0, NULL))< 0) { >>>>> error_report("error initializing"); >>>>> >>>> >>>> What about this patch? Can it be applied to the block branch? >>>> >>>> Regards, >>>> Stefan W. >>> >>> No objections on my side. You can add: >>> >>> Reviewed-by: Christian Brunner <chb@muc.de> >>> >>> The questions is how we continue with the rbd driver. Recent ceph >>> versions had some changes in librados that are incompatible with the >>> current driver. We have to options now: >>> >>> 1. Change the function calls for new librados versions (I could >>> provide a patch for this). >>> 2. Use librbd (see Josh's patches). >>> >>> Using librbd simplifies the qemu driver a lot and gives us consistency >>> with the kernel driver. - I would prefer this. (Please note that there >>> is a race condition in the current librbd versions, that crashes qemu >>> under high i/o load, but I'm fairly confident, that Josh will have >>> sorted this out by the time 0.15 is released). >> >> The problem with Josh's patches (or basically anything related to the >> rbd driver) is that it hasn't received any review. I'm not familiar with >> librados and librbd, so reviewing rbd is even harder than other patches >> of the same size for me. Additionally, it's not a test environment that >> I have set up. >> >> So going forward with it, I think we need a separate rbd maintainer. So >> Christian, I think it would be helpful if you at least reviewed any rbd >> patch and either comment on it or send an Acked-by, which basically >> tells me to commit it without any further checks. Or maybe we should >> consider that you send pull requests yourself if the patches touch only >> rbd code. >> >> Kevin > > This patch was reviewed by Christian, but is still in my queue of open > patches. > Kevin, could you please take it into the block queue? > > Thanks, > Stefan W. Kevin chose to merge josh's patches. This includes your patch. Christian
diff --git a/block/rbd.c b/block/rbd.c index 249a590..5c7d44e 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -524,7 +524,6 @@ static int rbd_open(BlockDriverState *bs, const char *filename, int flags) RbdHeader1 *header; char pool[RBD_MAX_SEG_NAME_SIZE]; char snap_buf[RBD_MAX_SEG_NAME_SIZE]; - char *snap = NULL; char *hbuf = NULL; int r; @@ -533,9 +532,6 @@ static int rbd_open(BlockDriverState *bs, const char *filename, int flags) s->name, sizeof(s->name)) < 0) { return -EINVAL; } - if (snap_buf[0] != '\0') { - snap = snap_buf; - } if ((r = rados_initialize(0, NULL)) < 0) { error_report("error initializing");
cppcheck report: rbd.c:246: style: Variable 'snap' is assigned a value that is never used Remove snap and the related code. Cc: Christian Brunner <chb@muc.de> Cc: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Stefan Weil <weil@mail.berlios.de> --- block/rbd.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)