On 2013-09-13 11:57, Kevin Wolf wrote:
[…]
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 “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. ;-)
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
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).
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
