From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35870) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wkj03-0007v9-Bl for qemu-devel@nongnu.org; Wed, 14 May 2014 19:54:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wkizy-0000OD-HB for qemu-devel@nongnu.org; Wed, 14 May 2014 19:54:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:23646) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wkizy-0000Nz-8n for qemu-devel@nongnu.org; Wed, 14 May 2014 19:54:10 -0400 Message-ID: <5374021D.6090309@redhat.com> Date: Thu, 15 May 2014 01:54:05 +0200 From: Max Reitz MIME-Version: 1.0 References: <1399926438-32292-1-git-send-email-ncmike@ncultra.org> In-Reply-To: <1399926438-32292-1-git-send-email-ncmike@ncultra.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] qemu-img fails to delete last snapshot List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mike Day , qemu-devel@nongnu.org Cc: kwolf@redhat.com, stefanha@redhat.com On 12.05.2014 22:27, Mike Day wrote: > When deleting the last snapshot, copying the resulting snapshot table > currently fails, causing the delete operation to also fail. Fix the > failure by skipping the copy and just writing the snapshot header and > freeing the extra clusters. > > There are two specific problems in the current code. First is a lack of > parenthesis in the calculation of the memmove size parameter: > > s->nb_snapshots - snapshot_index - 1 > > When s->nb_snapshots is 0, snapshot_index is 1. Before this patch is applied, s->nb_snapshots is only increased after=20 the memmove(). Therefore, it can never be 0 =96 snapshot_index on the=20 other hand needs to be 0, as find_snapshot_by_id_and_name() forces it to=20 be less than s->nb_snapshots (to elaborate on Kevin's review). > 0 - 1 - 1 =3D 0xfffffffe > > it should be: > > 0 - (1 - 1) =3D 0x00 > > The second problem is shifting the snapshot table to the left. After > removing the last snapshot there are no existing snapshots to be > shifted. All that needs to be done is to write the header and > unallocate the blocks. > > Signed-off-by: Mike Day > Reviewed-by: Eric Blake > --- > v2: improved the git log entry > added Eric Blake as a reviewer I do agree that this code is rather ugly and I had problems with it on=20 more than one occasion (which should be speaking for itself, considering=20 that I have not worked that long on qemu). On the other hand it is=20 always a nice test case whether one broke zero-size allocations, though=20 (while I'm not sure whether these should be allowed in the first place,=20 though=85). Considering that this code indeed does perform a zero-size allocation=20 reproducably, I'm rather surprised that we actually do not have a test=20 case yet for snapshot deletion, though (as far as I can see). So, all in all, I am kind of in favor of making the deletion of the last=20 snapshot a special case as this would probably greatly improve=20 readability; but on the other hand, it actually is a good test as it is=20 right now. Max