Re: [Qemu-devel] [PATCH 3/4] nbd/server: implement dirty bitmap export

[Vladimir Sementsov-Ogievskiy]

21.03.2018 19:57, Eric Blake wrote:
> On 03/21/2018 07:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>

[...]

> > +
> > +#define NBD_STATE_DIRTY 1
>
> Does this belong better in nbd.h, alongside NBD_STATE_HOLE?  (And 
> defining it as (1 << 0) might be nicer, too).

It's not used anywhere else, but it may be worth moving, for consistency 
and for future users. Ok, I'll move.


[...]

> >   +/* nbd_meta_bitmap_query
> > + *
> > + * Handle query to 'qemu-dirty-bitmap' namespace.
> > + * 'len' is the amount of text remaining to be read from the current 
> > name, after
> > + * the 'qemu-dirty-bitmap:' portion has been stripped.
> > + *
> > + * Return -errno on I/O error, 0 if option was completely handled by
> > + * sending a reply about inconsistent lengths, or 1 on success. */
> > +static int nbd_meta_bitmap_query(NBDClient *client, 
> > NBDExportMetaContexts *meta,
> > +                                 uint32_t len, Error **errp)
> > +{
> > +    if (!client->exp->export_bitmap) {
> > +        return nbd_opt_skip(client, len, errp);
> > +    }
> > +
> > +    return nbd_meta_single_query(client, 
> > client->exp->export_bitmap_name, len,
> > +                                 &meta->dirty_bitmap, errp);
>
> So if I'm reading this right, a client can do _LIST_ 
> 'qemu-dirty-bitmap:' (or 'qemu:dirty-bitmap:' if we choose a shorter 
> initial namespace) and get back the exported bitmap name; or the user 
> already knows the bitmap name and both _LIST_ and _SET_ 
> 'qemu-dirty-bitmap:name' return the one supported bitmap for that NBD 
> server.

yes

> >   /* nbd_co_send_extents
> > + *
> > + * NBD_REPLY_FLAG_DONE is not set, don't forget to send it.
> > + *
> >    * @extents should be in big-endian */
> >   static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
> >                                  NBDExtent *extents, unsigned 
> > nb_extents,
>
> Worth a bool flag that the caller can inform this function whether to 
> include FLAG_DONE?

It was simpler to just send it separately, after all BLOCK_STATUS 
replies. So, I didn't need it.

> > +
> > +static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
> > +                              BdrvDirtyBitmap *bitmap, uint64_t offset,
> > +                              uint64_t length, uint32_t context_id,
> > +                              Error **errp)
> > +{
> > +    int ret;
> > +    unsigned nb_extents;
> > +    NBDExtent *extents = g_new(NBDExtent, NBD_MAX_BITMAP_EXTENTS);
>
> Is this correct even when the client used NBD_CMD_FLAG_REQ_ONE?

Oops, looks like a bug.

--
Best regards,
Vladimir