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 <address@hidden>:
>> 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<address@hidden>
>>> Cc: Kevin Wolf<address@hidden>
>>> Signed-off-by: Stefan Weil<address@hidden>
>>> ---
>>>  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 <address@hidden>
> 
> 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