From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53577) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VUGjw-0003q9-0G for qemu-devel@nongnu.org; Thu, 10 Oct 2013 09:57:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VUGjo-0003yo-Py for qemu-devel@nongnu.org; Thu, 10 Oct 2013 09:57:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62020) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VUGjo-0003yR-H0 for qemu-devel@nongnu.org; Thu, 10 Oct 2013 09:57:12 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r9ADvBxT015937 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 10 Oct 2013 09:57:11 -0400 Date: Thu, 10 Oct 2013 15:57:04 +0200 From: Kevin Wolf Message-ID: <20131010135704.GH3046@dhcp-200-207.str.redhat.com> References: <1381395144-4449-1-git-send-email-mreitz@redhat.com> <1381395144-4449-2-git-send-email-mreitz@redhat.com> <20131010122625.GG3046@dhcp-200-207.str.redhat.com> <5256A38A.70407@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5256A38A.70407@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/2] qcow2: Undo leaked allocations in co_writev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 10.10.2013 um 14:54 hat Max Reitz geschrieben: > If the "currently" implied that this will turn out bad if there is a > new error condition between a successful call to > qcow2_alloc_cluster_link_l2 and the removal of the L2Meta request > from the list: Yes, that's true, of course. Yes, that's the scenario. It seems easy to miss the error handling path when reviewing a change to this code. > However, as you've said, > currently, there is no such condition; and I don't see why it should > be introduced. The sole purpose of the list seems to be (to me) to > execute qcow2_alloc_cluster_link_l2 on every of its elements. Thus, > as soon as qcow2_alloc_cluster_link_l2 is successful, the > corresponding request should be removed from the list. If anything isn't complex enough in qcow2, think about how things will turn out with Delayed COW and chances are that it does become complex. For example, you can then have other requests running in parallel which use the newly allocated cluster. You may decrease the refcount only after the last of them has completed. This is just the first case that comes to mind, I'm sure there's more fun to be had. > So, in case you do agree that it currently works fine, I would not > consider it risky; if this patch is applied and some time in the > future anything introduces a "goto fail" between > qcow2_alloc_cluster_link_l2 and l2_meta =3D next, this patch would > simply have to make sure that qcow2_free_clusters isn't called in > this case. In the probably very unlikely case all my previous > assumptions and conclusions were true, I'd just add a comment in the > qcow2_alloc_cluster_link_l2 loop informing about this case (=E2=80=9CIf= you > add a goto fail here, make sure to pay attention=E2=80=9D or something = along > these lines). Adding a comment there sounds like a fair compromise. > >Also, shouldn't it be QCOW2_DISCARD_OTHER? >=20 > I'm always unsure about the discard flags. ;-) >=20 > I try to follow the rule of =E2=80=9Cuse the specific type (or =E2=80=98= other=E2=80=99) for > freeing =E2=80=98out of the blue=E2=80=99, but use =E2=80=98always=E2=80= =99 if it's just a very > recent allocation that is being undone again=E2=80=9D. I'd gladly accep= t > better recommendations. ;-) To be honest, I'm not sure if there are any legitimate use cases for 'always'... Discard is a slow operation, so unless there's a specific reason anyway, I'd default to 'other' (or a specific type, of course). Kevin