From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53290) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VKQX0-0004Dp-2B for qemu-devel@nongnu.org; Fri, 13 Sep 2013 06:23:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VKQWs-0002en-Pp for qemu-devel@nongnu.org; Fri, 13 Sep 2013 06:23:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57222) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VKQWs-0002ei-IM for qemu-devel@nongnu.org; Fri, 13 Sep 2013 06:23:10 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8DAN9Pf008377 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 13 Sep 2013 06:23:09 -0400 Message-ID: <5232E78A.2030406@redhat.com> Date: Fri, 13 Sep 2013 12:23:06 +0200 From: Max Reitz MIME-Version: 1.0 References: <1378106712-29856-1-git-send-email-mreitz@redhat.com> <5231D673.6050907@redhat.com> <5231DCA6.7090004@redhat.com> <20130913095753.GC2804@dhcp-200-207.str.redhat.com> In-Reply-To: <20130913095753.GC2804@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 0/8] Add metadata overlap checks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi On 2013-09-13 11:57, Kevin Wolf wrote: > > [=85] > Am 12.09.2013 um 17:24 hat Max Reitz geschrieben: >> On the other hand, we could replace QCOW2_OL_DEFAULT manually >> everywhere by s->default_overlap_check; or, we change the macro to >> take s as a parameter. >> >> Either way, I see three answers to your question: >> >> First, right now, we're basically stuck with a compile time constant. >> >> Second, if a user really wants to, he could always define the macro >> to represent some variable. This should work pretty well already, >> although this variable has to be defined first, of course. >> >> Third, it wouldn't be too much of a problem to write a follow-up >> patch manually replacing every QCOW2_OL_DEFAULT occurrence by a true >> variable (such as default_overlap_check from the current >> BDRVQcowState structure). I'd just undefine the macro and work down >> every compiler error. ;-) >> >> On the other hand, now, that I think about it, we could also invert >> the current program logic: qcow2_check_metadata_overlap would then >> no longer receive a bitmask of structures to check for overlaps, but >> rather a bitmask of structures to ignore, since it can figure out >> s->default_overlap_check by itself. > Yup, see above. It's not necessary to invert to logic, though it's an > option and perhaps the nicer one. The crucial part is moving the > evalution of s->overlap_check into qcow2_check_metadata_overlap. (Note > how I dropped the 'default' from s->default_overlap_check, it's not a > default any more, but the actual user choice.) Well, I didn't mean the =93default=94 to express that it's QEMU's default= ,=20 but rather that it's the default check to be performed by=20 qcow2_check_metadata_overlap if the caller doesn't have any=20 =93restrictions=94 (such as which structures should be ignored). But anyw= ay,=20 it makes sense to drop the prefix in this case as well, because that=20 value will then be used inside qcow2_check_metadata_overlap alone.=20 Furthermore, I'm always in favor of shorter variable names. ;-) > The more interesting part is that adding an option always needs thought > because once it is exposed, it's an API that is set in stone. And I'm > also not sure what the best command line and QMP representations of a > bitmask like this are. I'd personally add it to the runtime options of qcow2. In addition, I=20 propose we add a mechanism to generally amend runtime options at runtime=20 through QMP (if there isn't one already which I then am unaware of). I=20 don't see why we should just allow the kind of overlap checks performed=20 to be changed at runtime, but not, for instance, whether lazy refcounts=20 should be used (except the latter would be a bit harder to implement, I=20 guess). About the representation: The discard behavior is basically a bitfield=20 already and gives us therefore one possible representation (which is,=20 just using a single boolean per structure, named something like=20 "overlap-check.active-l1" etc.). In QMP we could probably also use a=20 dict, but then again, this is a decision to be made when generally=20 allowing modification of the qcow2 runtime options through QMP (in my=20 opinion). And finally, we could obviously just use an integer to=20 represent the mask. I think, we should first take care of the command line interface and=20 about QMP later (that is, if you agree on generally allowing=20 modification of the qcow2 runtime options through QMP). There, we could=20 offer both one boolean per mask element and an integer option, probably=20 the boolean flags taking precedence. The flags are nice for users who want an "easily" comprehensible=20 interface, the masked integer is better for those who prefer a short=20 representation. Max