Re: [PATCH v4 5/9] blockdev: Split off basic bitmap operations for qemu-img

[Eric Blake]

On 5/14/20 1:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.05.2020 04:16, Eric Blake wrote:
> > Upcoming patches want to add some basic bitmap manipulation abilities
> > to qemu-img.  But blockdev.o is too heavyweight to link into qemu-img
> > (among other things, it would drag in block jobs and transaction
> > support - qemu-img does offline manipulation, where atomicity is less
> > important because there are no concurrent modifications to compete
> > with), so it's time to split off the bare bones of what we will need
> > into a new file block/monitor/bitmap-qmp-cmds.o.
> >
> > This is sufficient to expose 6 QMP commands for use by qemu-img (add,
> > remove, clear, enable, disable, merge), as well as move the three
> > helper functions touched in the previous patch.  Regarding
> > MAINTAINERS, the new file is automatically part of block core, but
> > also makes sense as related to other dirty bitmap files.
> >
> > Signed-off-by: Eric Blake <address@hidden>
> > Reviewed-by: Max Reitz <address@hidden>
> > ---

> > +++ b/block/monitor/bitmap-qmp-cmds.c
>
> Hmm, shouldn't transaction bitmap actions be moved here too? May be, not 
> in these series..

No, the very reason that we split this file off of blockdev.c is that 
transactions are NOT needed in qemu-img.  You have to have separate .o 
files for what qemu-img needs, vs. what the rest of qemu proper needs, 
and transactions are only needed in qemu proper at the moment.  If we 
ever need transactions in Kevin's qemu-storage-daemon but not all of 
blockdev.c, then we'd need yet another .c file independent from either 
blockdev.c or this patch's new bitmap-qmp-cmds.c.

> > @@ -0,0 +1,323 @@
> > +/*
> > + * QEMU host block device bitmaps
>
> A bit conflicts with tha fact that they are not of block-device level 
> but of node-level.
>
> May be just "Block dirty bitmap qmp commands" ?

Copy-and-paste from blockdev.c, tweaked by adding one word.  Your 
wording is also fine.

> > + *
> > + * Copyright (c) 2003-2008 Fabrice Bellard
>
> Does it really apply here? block-dirty-bitmap-add was added in 2015.. 
> May be Red Hat and Virtuozzo copyrights would be more appropriate.

When splitting a file, the safest course of action is to preserve ALL 
copyright from the original file into the new file.

Adding additional copyright lines is okay as part of submitting new 
functionality, but in this case, I don't feel comfortable adding Red Hat 
copyright (my split isn't adding new functionality), and I don't have 
authorization to add Virtuozzo copyright (as that is not my employer).


> > +#include "qemu/osdep.h"
> > +
> > +#include "sysemu/blockdev.h"
> > +#include "block/block.h"
> > +#include "block/block_int.h"
> > +#include "qapi/qapi-commands-block.h"
> > +#include "qapi/error.h"
>
> compiles for with only four:
>
>    #include "qemu/osdep.h"
>    #include "block/block_int.h"
>    #include "qapi/qapi-commands-block.h"
>    #include "qapi/error.h"

Thanks. I blame rebase; an earlier version used a different header in 
patch 4/9; when I moved things to block_int.h in that patch, I didn't 
realize that this patch could be improved.

> with at least extra includes dropped:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

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