From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42269) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QQ1pV-0004MJ-7P for qemu-devel@nongnu.org; Fri, 27 May 2011 14:32:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QQ1pT-0000ZL-M5 for qemu-devel@nongnu.org; Fri, 27 May 2011 14:32:13 -0400 Received: from moutng.kundenserver.de ([212.227.17.9]:56232) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QQ1pT-0000ZB-AC for qemu-devel@nongnu.org; Fri, 27 May 2011 14:32:11 -0400 Message-ID: <4DDFEE25.60502@mail.berlios.de> Date: Fri, 27 May 2011 20:32:05 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1304799357-19281-1-git-send-email-weil@mail.berlios.de> <4DD8FC9D.3090200@mail.berlios.de> <4DDA3646.404@redhat.com> In-Reply-To: <4DDA3646.404@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block/rbd: Remove unused local variable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: chb@muc.de, QEMU Developers Am 23.05.2011 12:26, schrieb Kevin Wolf: > Am 23.05.2011 11:01, schrieb Christian Brunner: >> 2011/5/22 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 >>>> Cc: Kevin Wolf >>>> Signed-off-by: Stefan Weil >>>> --- >>>> 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 >> >> 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.