Re: [PATCH 04/17] block: Improve documentation of .bdrv_has_zero_init

[Eric Blake]

On 2/4/20 9:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> 31.01.2020 20:44, Eric Blake wrote:
> > Several drivers supply .bdrv_has_zero_init that returns 1, but lack
> > the .bdrv_has_zero_init_truncate callback (parallels and qed outright,
> > vdi in some scenarios).  A literal reading of the existing
> > documentation says such drivers are broken, because
> > bdrv_has_zero_init_truncate() defaults to zero if the callback is
> > missing; but in practice, the tie between the two functions is only
> > relevant when truncate is supported.  Clarify the documentation to
> > make it obvious that this is okay.
> >
> > Fixes: 1dcaf527
> > Signed-off-by: Eric Blake <address@hidden>
> > ---
> >   include/block/block_int.h | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 640fb82c789e..77ab45dc87cf 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -444,7 +444,8 @@ struct BlockDriver {
> >       /*
> >        * Returns 1 if newly created images are guaranteed to contain only
> >        * zeros, 0 otherwise.
> > -     * Must return 0 if .bdrv_has_zero_init_truncate() returns 0.
> > +     * Must return 0 if .bdrv_co_truncate is set and
> > +     * .bdrv_has_zero_init_truncate() returns 0.
> >        */
> >       int (*bdrv_has_zero_init)(BlockDriverState *bs);
>
> I doubt, shouldn't 1dcaf527 be fixed by adding all needed 
> bdrv_has_zero_init_truncate functions?

That was my original thought. But looking at callers of 
bdrv_has_zero_init_truncate() shows that they all plan to perform 
bdrv_co_truncate(PREALLOC_MODE_OFF) and want to know if that will leak 
non-zero data; if you can't truncate, it doesn't matter what 
init_truncate() returns, but since init_truncate() defaults to 0, it 
shouldn't invalidate init() returning 1 - fixing the docs was easier 
than adding useless callbacks to parallels, qed, and vdi just to rip 
them back out again in patch 9.

As you noted later, sheepdog was the only driver that violated this rule 
(and it is fixed in patch 8).  I could reorder the series to get the bug 
fix in before the documentation change, if that would help.

> (sorry, I started to dig in the code and patches around 
> bdrv_has_zero_init_truncate and tired :(.,
>   hope Max will comment this).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org