From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51419) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bvSfk-0007Id-VK for qemu-devel@nongnu.org; Sat, 15 Oct 2016 13:23:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bvSfj-0002zh-P9 for qemu-devel@nongnu.org; Sat, 15 Oct 2016 13:23:00 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:35331 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 1bvSfj-0002yv-Ct for qemu-devel@nongnu.org; Sat, 15 Oct 2016 13:22:59 -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> <5801279A.6010403@virtuozzo.com> <4f380514-85b2-e261-fd4d-da81867f2369@redhat.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <580265E3.8080003@virtuozzo.com> Date: Sat, 15 Oct 2016 20:22:43 +0300 MIME-Version: 1.0 In-Reply-To: <4f380514-85b2-e261-fd4d-da81867f2369@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 15.10.2016 20:03, Max Reitz wrote: > On 14.10.2016 20:44, Vladimir Sementsov-Ogievskiy wrote: >> 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.. > Yes, I know that the loop is safe in the sense of the word "safe", but I > meant that it's kind of confusing that the loop uses > "for_each_bitmap_dir_entry_unsafe" and thus is "unsafe", too. > > Another idea would be to write "This is actually safe" instead of "loop > is safe". Finally I've decided to introduce normal list of normal structures like in snapshots.. > >>>> + * 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. > error_prepend() would be the function. > > This code will be invoked by any code that is opening an image. There > are of course a couple of places where that can be the case: For > external tools such as qemu-img, it's normally pretty clear which image > is meant (and it additionally uses error_reportf_err() to give you more > information). > > For -drive, every error message will at least be prepended by the > corresponding -drive parameter, so that will help a bit. > > blockdev-add, unfortunately, doesn't do anything like this. But the user > can of course choose to construct the BDS graph node by node and thus > always know which node an error originates from. > > Anyway, if you want to add this information to every error message, you > should probably do so in bdrv_open_inherit(). The issue I'd take with > this is that most users probably won't set the node name themselves > (either they don't at all or it's some management tool that does), so > reporting the node name doesn't actually help them at all (and > management tools are not supposed to parse error messages, so it won't > help in that case either). > > Max > Thanks for explanations, and for the whole review, it's great! Sorry for my laziness and for spelling( O! I've discovered vim spell, so one problem less, I hope. -- Best regards, Vladimir