|
From: | Max Reitz |
Subject: | Re: [Qemu-devel] [PATCH v5 0/8] Add metadata overlap checks |
Date: | Fri, 13 Sep 2013 12:23:06 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130805 Thunderbird/17.0.8 |
On 2013-09-13 11:57, Kevin Wolf wrote:
Well, I didn't mean the “default” to express that it's QEMU's default, but rather that it's the default check to be performed by qcow2_check_metadata_overlap if the caller doesn't have any “restrictions” (such as which structures should be ignored). But anyway, it makes sense to drop the prefix in this case as well, because that value will then be used inside qcow2_check_metadata_overlap alone. Furthermore, I'm always in favor of shorter variable names. ;-)[…] 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.)
I'd personally add it to the runtime options of qcow2. In addition, I propose we add a mechanism to generally amend runtime options at runtime through QMP (if there isn't one already which I then am unaware of). I don't see why we should just allow the kind of overlap checks performed to be changed at runtime, but not, for instance, whether lazy refcounts should be used (except the latter would be a bit harder to implement, I guess).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.
About the representation: The discard behavior is basically a bitfield already and gives us therefore one possible representation (which is, just using a single boolean per structure, named something like "overlap-check.active-l1" etc.). In QMP we could probably also use a dict, but then again, this is a decision to be made when generally allowing modification of the qcow2 runtime options through QMP (in my opinion). And finally, we could obviously just use an integer to represent the mask.
I think, we should first take care of the command line interface and about QMP later (that is, if you agree on generally allowing modification of the qcow2 runtime options through QMP). There, we could offer both one boolean per mask element and an integer option, probably the boolean flags taking precedence.
The flags are nice for users who want an "easily" comprehensible interface, the masked integer is better for those who prefer a short representation.
Max
[Prev in Thread] | Current Thread | [Next in Thread] |