Re: [Qemu-devel] [PATCH v5 0/8] Add metadata overlap checks

[Max Reitz <mreitz@redhat.com>]

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