Re: [Qemu-devel] [PATCH RESEND 41/50] blockdev: Add blockdev-insert-medium

[Max Reitz]

On 2015-01-28 at 15:18, Eric Blake wrote:
> On 01/27/2015 12:46 PM, Max Reitz wrote:
> > And a helper function for that which directly takes a pointer to the BDS
> > to be inserted instead of its node-name (which will be used for
> > implementing 'change' using blockdev-insert-medium).
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   blockdev.c           | 43 +++++++++++++++++++++++++++++++++++++++++++
> >   qapi/block-core.json | 17 +++++++++++++++++
> >   qmp-commands.hx      | 38 ++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 98 insertions(+)
> >
> > +static void qmp_blockdev_insert_anon_medium(const char *device,
> > +                                            BlockDriverState *bs, Error **errp)
> > +{
> > +    BlockBackend *blk;
> > +
> > +    blk = blk_by_name(device);
> > +    if (!blk) {
> > +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> > +        return;
> > +    }
> > +
> > +    if (!blk_dev_has_removable_media(blk)) {
> > +        error_setg(errp, "Device '%s' is not removable", device);
> > +        return;
> > +    }
> > +
> > +    if (!blk_dev_is_tray_open(blk)) {
> > +        error_setg(errp, "Tray of device '%s' is not open", device);
> > +        return;
> > +    }
> > +
> > +    if (blk_bs(blk)) {
> > +        error_setg(errp, "There already is a medium in device '%s'", device);
> > +        return;
> > +    }
> Good, you didn't implement hot-swap semantics of replacing an existing
> medium (that gets too confusing; so I _like_ that you force the user to
> consider all the steps through multiple low-level commands).
>
>
> > +
> > +Example (1):
> I'll quit pointing it out; but if you clean up the useless (1) in one
> patch, do it across the series.

Yes, will do.

> > +
> > +-> { "execute": "blockdev-add",
> > +     "arguments": { "options": { "id": "backend0",
> > +                                 "node-name": "node0",
> Why is 'id' needed?

Oops, because I forgot removing it after I added the functionality for 
creating a BDS tree without a BB. I'll remove it, thanks for catching it.

Max

> Isn't the point of this command sequence to create
> a BDS tree that is NOT tied to a BB, and then use insert-medium to make
> the association after the fact?  We don't need to create a BB named
> 'backend0' if we are immediately going to reuse 'node0' in a different
> BB (true, we have somewhat anticipated the idea of sharing BDS tree
> among multiple BB, but haven't quite turned that on before now).
>
> > +                                 "driver": "raw",
> > +                                 "file": { "driver": "file",
> > +                                           "filename": "fedora.iso" } } } }
> > +
> > +<- { "return": {} }
> > +
> > +-> { "execute": "blockdev-insert-medium",
> > +     "arguments": { "device": "ide1-cd0",
> > +                    "node-name": "node0" } }
> > +
> > +<- { "return": {} }
> If you can either explain why you used 'id' in the example, or remove
> that parameter, you can add:
> Reviewed-by: Eric Blake <address@hidden>