Re: [Qemu-devel] [PATCH 25/25] hmp: add NBD server commands

[Paolo Bonzini <pbonzini@redhat.com>]

> Why's nbd_server_add not needed in HMP?
> 
> Oh, it's because HMP's nbd_server_start auto-adds *all* block
> devices, unlinke QMP's nbd-server-start.
> 
> Are you sure that's a good idea?

Yes.  Now that we have QMP we can go back and treat HMP as (mostly) a
debugging interface as it was meant to be.

You don't need any fine-grained control in that case.  From the server's
point of view, serving all devices or just one is exactly the same thing.
If you use NBD for block migration, for example, you can choose _on the
source_ which devices you are migrating, but it doesn't harm if more
devices are exposed on the target.

The only case where this loses is when you hotplug a device and you want
to start serving it.  I think this is a minor loss for HMP, but it is a
show-stopper for QMP which hence has to have nbd-server-add.

Paolo

> >  #if defined(TARGET_I386)
> >  
> >      {
> > diff --git a/hmp.c b/hmp.c
> > index 70bdec2..edf6397 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -18,6 +18,7 @@
> >  #include "qemu-option.h"
> >  #include "qemu-timer.h"
> >  #include "qmp-commands.h"
> > +#include "qemu_socket.h"
> >  #include "monitor.h"
> >  #include "console.h"
> >  
> > @@ -1209,3 +1210,57 @@ void hmp_screen_dump(Monitor *mon, const
> > QDict *qdict)
> >      qmp_screendump(filename, &err);
> >      hmp_handle_error(mon, &err);
> >  }
> > +
> > +void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
> > +{
> > +    const char *uri = qdict_get_str(qdict, "uri");
> > +    int writable = qdict_get_try_bool(qdict, "writable", 0);
> > +    Error *local_err = NULL;
> > +    BlockInfoList *block_list, *info;
> > +    SocketAddress *addr;
> > +
> > +    /* First check if the address is valid and start the server.
> >  */
> > +    addr = socket_parse(uri, &local_err);
> > +    if (local_err != NULL) {
> > +        goto exit;
> > +    }
> > +
> > +    qmp_nbd_server_start(addr, &local_err);
> > +    qapi_free_SocketAddress(addr);
> > +    if (local_err != NULL) {
> > +        goto exit;
> > +    }
> > +
> > +    /* Then try adding all block devices.  If one fails, close all
> > and
> > +     * exit.
> > +     */
> > +    block_list = qmp_query_block(NULL);
> > +
> > +    for (info = block_list; info; info = info->next) {
> > +        if (!info->value->has_inserted) {
> > +            continue;
> > +        }
> > +
> > +        qmp_nbd_server_add(info->value->device,
> > +                           true, !info->value->inserted->ro &&
> > writable,
> > +                           &local_err);
> > +
> > +        if (local_err != NULL) {
> > +            qmp_nbd_server_stop(NULL);
> > +            break;
> > +        }
> > +    }
> 
> Here's where it auto-adds block devices.
> 
> > +
> > +    qapi_free_BlockInfoList(block_list);
> > +
> > +exit:
> > +    hmp_handle_error(mon, &local_err);
> > +}
> > +
> > +void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *errp = NULL;
> > +
> > +    qmp_nbd_server_stop(&errp);
> > +    hmp_handle_error(mon, &errp);
> > +}
> > diff --git a/hmp.h b/hmp.h
> > index 71ea384..45b7c48 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -75,5 +75,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
> >  void hmp_closefd(Monitor *mon, const QDict *qdict);
> >  void hmp_send_key(Monitor *mon, const QDict *qdict);
> >  void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> > +void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
> > +void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
> >  
> >  #endif
>