From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41654) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bv7Tc-0003hF-02 for qemu-devel@nongnu.org; Fri, 14 Oct 2016 14:45:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bv7Ta-0006cE-0H for qemu-devel@nongnu.org; Fri, 14 Oct 2016 14:45:02 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:9526 helo=relay.sw.ru) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bv7TZ-0006bh-Ks for qemu-devel@nongnu.org; Fri, 14 Oct 2016 14:45:01 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-8-git-send-email-vsementsov@virtuozzo.com> <93be46ce-5b73-c7d3-ada0-d59461227dbf@redhat.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <5801279A.6010403@virtuozzo.com> Date: Fri, 14 Oct 2016 21:44:42 +0300 MIME-Version: 1.0 In-Reply-To: <93be46ce-5b73-c7d3-ada0-d59461227dbf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com On 01.10.2016 19:26, Max Reitz wrote: > On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >> Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are ... > + goto fail; > + } > + > + /* loop is safe because next entry offset is calculated after conversion to > Should it be s/safe/unsafe/? loop is safe. _unsafe is related to absence of assert in a loop, as it loops through BE data. Bad wording, I'll change it somehow.. > >> + * cpu format */ >> + for_each_bitmap_dir_entry_unsafe(e, dir, size) { >> + if ((uint8_t *)(e + 1) > dir_end) { >> + goto broken_dir; >> + } >> + >> + bitmap_dir_entry_to_cpu(e); >> + >> + if ((uint8_t *)next_dir_entry(e) > dir_end) { >> + goto broken_dir; >> + } >> + >> + if (e->extra_data_size != 0) { >> + error_setg(errp, "Can't load bitmap '%.*s' from '%s':" >> + "extra data not supported.", e->name_size, > Full stop again. > > Also, I'm not quite sure why you give the device/node name here. You > don't do that anywhere else and I think if we want to emit the > information where something failed, it should be added at some previous > point in the call chain. For example, how? As I understand, I can emit it only by error_setg, so actually it would be better to add node and bitmap name to all error_setg in the code.. or create helper function for it. > >> + dir_entry_name_notcstr(e), >> + bdrv_get_device_or_node_name(bs)); >> + goto fail; >> + } >> + } ... >> + if (ret < 0) { >> + goto fail; >> + } >> + } >> + s->bitmap_directory_offset = new_offset; >> + s->bitmap_directory_size = new_size; >> + s->nb_bitmaps = new_nb_bitmaps; >> + >> + ret = update_header_sync(bs); >> + if (ret < 0) { >> + goto fail; >> + } >> + >> + if (old_size) { >> + qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS); >> + } >> + >> + return 0; >> + >> +fail: >> + if (new_offset > 0) { >> + qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS); >> + s->bitmap_directory_offset = old_offset; >> + s->bitmap_directory_size = old_size; >> + s->nb_bitmaps = old_nb_bitmaps; >> + s->autoclear_features = old_autocl; > Why are you restoring the autoclear features? From a quick glance I > can't see any code path that changes this field here, and if there is > one, it probably has a good reason to do so. hmm.. it's an artefact from future). should be moved to later patch. -- Best regards, Vladimir