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

[Kevin Wolf <kwolf@redhat.com>]

Am 12.09.2013 um 17:24 hat Max Reitz geschrieben:
> On 2013-09-12 16:57, Eric Blake wrote:
> >Also, is there a way to tune the set of checks used at runtime, or are
> >we stuck with the compiled-in default?  That is, can a user opt in to
> >more expensive tests for robustness, or opt out of default tests for
> >speed, via a runtime command, or is it something where they have to
> >recompile to choose a different QCOW2_OL_DEFAULT value?
> >
> Right now, it's obviously a compile-time constant. However, I think
> we could also define QCOW2_OL_DEFAULT to be a runtime variable,
> however, that variable would most probably be stored in the
> BDRVQcowState structure. We could take advantage of that structure's
> instance being named "s" almost universally, therefore we could
> define QCOW2_OL_DEFAULT to be s->default_overlap_check or something
> like that. It would obviously be kind of ugly, but it should work
> pretty well.

Not really acceptable, and not necessary either.

You could change all qcow2_pre_write_overlap_check() caller to not pass
QCOW2_OL_DEFAULT & foo, but pass either just foo or QCOW2_OL_ALL & ~foo,
and apply a s->overlap_checks mask inside qcow2_pre_write_overlap_check().

> 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.)

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.

Kevin