[Qemu-devel] [PATCH] block/rbd: Remove unused local variable

[Qemu-devel] [PATCH] block/rbd: Remove unused local variable

[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");
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH] block/rbd: Remove unused local variable

[Stefan Weil]

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.

Re: [Qemu-devel] [PATCH] block/rbd: Remove unused local variable

[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).

Regards,
Christian

Re: [Qemu-devel] [PATCH] block/rbd: Remove unused local variable

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH] block/rbd: Remove unused local variable

[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

Re: [Qemu-devel] [PATCH] block/rbd: Remove unused local variable

[Stefan Weil]

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.

Re: [Qemu-devel] [PATCH] block/rbd: Remove unused local variable

[Christian Brunner]

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

[Qemu-devel] [PATCH v2] block/rbd: Remove unused local variable

[Stefan Weil]

Variable 'snap' is assigned a value that is never used.
Remove snap and the related code.

v2:
  The unused variable which was in function rbd_open is now in function
  qemu_rbd_create, so the patch needed an update.

Cc: Christian Brunner <chb@muc.de>
Cc: Josh Durgin <josh.durgin@dreamhost.com>
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 bdc448a..d5659cd 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -227,7 +227,6 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options)
     char name[RBD_MAX_IMAGE_NAME_SIZE];
     char snap_buf[RBD_MAX_SNAP_NAME_SIZE];
     char conf[RBD_MAX_CONF_SIZE];
-    char *snap = NULL;
     rados_t cluster;
     rados_ioctx_t io_ctx;
     int ret;
@@ -238,9 +237,6 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options)
                            conf, sizeof(conf)) < 0) {
         return -EINVAL;
     }
-    if (snap_buf[0] != '\0') {
-        snap = snap_buf;
-    }
 
     /* Read out options */
     while (options && options->name) {
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v2] block/rbd: Remove unused local variable

[Josh Durgin]

On 06/10/2011 01:05 PM, Stefan Weil wrote:
> Variable 'snap' is assigned a value that is never used.
> Remove snap and the related code.
>
> v2:
>    The unused variable which was in function rbd_open is now in function
>    qemu_rbd_create, so the patch needed an update.
>
> Cc: Christian Brunner<chb@muc.de>
> Cc: Josh Durgin<josh.durgin@dreamhost.com>
> 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 bdc448a..d5659cd 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -227,7 +227,6 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options)
>       char name[RBD_MAX_IMAGE_NAME_SIZE];
>       char snap_buf[RBD_MAX_SNAP_NAME_SIZE];
>       char conf[RBD_MAX_CONF_SIZE];
> -    char *snap = NULL;
>       rados_t cluster;
>       rados_ioctx_t io_ctx;
>       int ret;
> @@ -238,9 +237,6 @@ static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options)
>                              conf, sizeof(conf))<  0) {
>           return -EINVAL;
>       }
> -    if (snap_buf[0] != '\0') {
> -        snap = snap_buf;
> -    }
>
>       /* Read out options */
>       while (options&&  options->name) {

Looks good to me:

Reviewed-by: Josh Durgin <josh.durgin@dreamhost.com>

Re: [Qemu-devel] [PATCH v2] block/rbd: Remove unused local variable

[Kevin Wolf]

Am 10.06.2011 22:05, schrieb Stefan Weil:
> Variable 'snap' is assigned a value that is never used.
> Remove snap and the related code.
> 
> v2:
>   The unused variable which was in function rbd_open is now in function
>   qemu_rbd_create, so the patch needed an update.
> 
> Cc: Christian Brunner <chb@muc.de>
> Cc: Josh Durgin <josh.durgin@dreamhost.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>

Thanks, applied to the block branch.

Kevin