From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52648) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqRpE-0002Qr-G9 for qemu-devel@nongnu.org; Mon, 17 Nov 2014 14:19:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XqRp8-0002TC-BY for qemu-devel@nongnu.org; Mon, 17 Nov 2014 14:19:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45013) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqRp8-0002Sw-4k for qemu-devel@nongnu.org; Mon, 17 Nov 2014 14:18:54 -0500 Message-ID: <546A4A14.3040001@redhat.com> Date: Mon, 17 Nov 2014 14:18:44 -0500 From: John Snow MIME-Version: 1.0 References: 1414639364-4499-6-git-send-email-famz@redhat.com <545CE25B.9030208@parallels.com> In-Reply-To: <545CE25B.9030208@parallels.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , famz@redhat.com Cc: Kevin Wolf , Benoit Canet , Markus Armbruster , qemu-devel@nongnu.org, Max Reitz , Stefan Hajnoczi , Jd , Paolo Bonzini , Luiz Capitulino On 11/07/2014 10:16 AM, Vladimir Sementsov-Ogievskiy wrote: > from [PATCH v6 02/10] >> +void qmp_block_dirty_bitmap_remove(const char *device, const char *name, >> + Error **errp) >> +{ >> + BlockDriverState *bs; >> + BdrvDirtyBitmap *bitmap; >> + >> + bs = bdrv_find(device); >> + if (!bs) { >> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >> + return; >> + } >> + >> + if (!name || name[0] == '\0') { >> + error_setg(errp, "Bitmap name cannot be empty"); >> + return; >> + } >> + bitmap = bdrv_find_dirty_bitmap(bs, name); >> + if (!bitmap) { >> + error_setg(errp, "Dirty bitmap not found: %s", name); >> + return; >> + } >> + >> + bdrv_dirty_bitmap_make_anon(bs, bitmap); >> + bdrv_release_dirty_bitmap(bs, bitmap); >> +} > > from [PATCH v6 05/10]: >> +void qmp_block_dirty_bitmap_enable(const char *device, const char *name, >> + Error **errp) >> +{ >> + BlockDriverState *bs; >> + BdrvDirtyBitmap *bitmap; >> + >> + bs = bdrv_find(device); >> + if (!bs) { >> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >> + return; >> + } >> + >> + bitmap = bdrv_find_dirty_bitmap(bs, name); >> + if (!bitmap) { >> + error_setg(errp, "Dirty bitmap not found: %s", name); >> + return; >> + } >> + >> + bdrv_enable_dirty_bitmap(bs, bitmap); >> +} >> + >> +void qmp_block_dirty_bitmap_disable(const char *device, const char *name, >> + Error **errp) >> +{ >> + BlockDriverState *bs; >> + BdrvDirtyBitmap *bitmap; >> + >> + bs = bdrv_find(device); >> + if (!bs) { >> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >> + return; >> + } >> + >> + bitmap = bdrv_find_dirty_bitmap(bs, name); >> + if (!bitmap) { >> + error_setg(errp, "Dirty bitmap not found: %s", name); >> + return; >> + } >> + >> + bdrv_disable_dirty_bitmap(bs, bitmap); >> +} >> + > > there is one inconsistence: > > you have check >> + if (!name || name[0] == '\0') { >> + error_setg(errp, "Bitmap name cannot be empty"); >> + return; >> + } > when accessing bitmap in qmp_block_dirty_bitmap_remove, but not in > qmp_block_dirty_bitmap_{enable,disable}. In qmp_block_dirty_bitmap_{enable,disable} I don't think it's a problem if we don't check the name here -- it just means that we're not going to be able to find the name (since it's invalid) and the function will error out anyway. I don't think a change is warranted, necessarily, unless we factor out the error checking: see below. > Also, I think it'll be better to put similar part of these three > functions into one separate function to avoid duplicates, like > > static BdrvDirtyBitmap *bitmap find_dirty_bitmap(const char *device, const char *name, > Error **errp) > { > BlockDriverState *bs; > BdrvDirtyBitmap *bitmap; > > // most simple error condition earlier > if (!name || name[0] == '\0') { > error_setg(errp, "Bitmap name cannot be empty"); > return NULL; > } > > bs = bdrv_find(device); > if (!bs) { > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return NULL; > } > > bitmap = bdrv_find_dirty_bitmap(bs, name); > if (!bitmap) { > error_setg(errp, "Dirty bitmap not found: %s", name); > return NULL; > } > > return bitmap; > } > I think normally I would agree, but since qmp_block_dirty_bitmap_remove needs to use the BlockDriverState anyway, part of this error-checking routine gets duplicated regardless, unless you add another return parameter to give you the BlockDriverState back as well, and you lose the single purpose nature of factoring it out. I will keep the redundancy of the error-checking of this patch in mind as I clean it up for v7... > -- > Best regards, > Vladimir >