| Message ID | 1349877786-23514-26-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Paolo Bonzini <pbonzini@redhat.com> writes: > Acked-by: Luiz Capitulino <lcapitulino@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hmp-commands.hx | 29 +++++++++++++++++++++++++++++ > hmp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > hmp.h | 2 ++ > 3 file modificati, 86 inserzioni(+) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index e0b537d..f5d9204 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -1274,6 +1274,35 @@ Remove all matches from the access control list, and set the default > policy back to @code{deny}. > ETEXI > > + { > + .name = "nbd_server_start", > + .args_type = "writable:-w,uri:s", > + .params = "nbd_server_start [-w] host:port", > + .help = "serve block devices on the given host and port", > + .mhandler.cmd = hmp_nbd_server_start, > + }, > +STEXI > +@item nbd_server_start @var{host}:@var{port} > +@findex nbd_server_start > +Start an NBD server on the given host and/or port, and serve all of the > +virtual machine's block devices that have an inserted media on it. > +The @option{-w} option makes the devices writable. > +ETEXI > + > + { > + .name = "nbd_server_stop", > + .args_type = "", > + .params = "nbd_server_stop", > + .help = "stop serving block devices using the NBD protocol", > + .mhandler.cmd = hmp_nbd_server_stop, > + }, > +STEXI > +@item nbd_server_stop > +@findex nbd_server_stop > +Stop the QEMU embedded NBD server. > +ETEXI > + > + 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? > #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
> 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 >
On Fri, 19 Oct 2012 04:58:45 -0400 (EDT) Paolo Bonzini <pbonzini@redhat.com> wrote: > > > 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. We can add it later case we regret, so I'd vote for the simpler interface for now.
Paolo Bonzini <pbonzini@redhat.com> writes: >> 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. Apropos hotplug. The only way to unexport a block device is to stop the NBD server outright. Once the device backend has been exported, unplugging the device gets rid of the frontend, but the backend stays until you stop the NBD server, or you kill the backend with the big drive_del hammer. Makes me wonder whether we need QMP command nbd-server-del. Back to HMP. I agree that we can the human monitor is again for human users only, thus we don't have to encumber it with the need of tools. I might also buy your argument that extra exports don't really hurt. But device plug / -unplug is just as relevant there as in QMP. If it's a show-stopper for QMP, then I don't think it can be a "minor loss" for HMP. Additionally, two details I don't like: 1. The HMP command looks just like the QMP command (name & arguments), yet does something else. 2. What if we ever change our minds regarding fine-grained NBD export control in the human monitor? Add a second command to start the server without exporting everything? Add a "don't auto-export" argument to the existing command?
Il 19/10/2012 14:44, Markus Armbruster ha scritto: > Apropos hotplug. The only way to unexport a block device is to stop the > NBD server outright. Once the device backend has been exported, > unplugging the device gets rid of the frontend, but the backend stays > until you stop the NBD server, or you kill the backend with the big > drive_del hammer. Right. (Though for removable devices you can just eject it, which calls the close notifier). > Makes me wonder whether we need QMP command nbd-server-del. Perhaps yes, but it can be added later. > I agree that we can the human monitor is again for human users only, > thus we don't have to encumber it with the need of tools. I might also > buy your argument that extra exports don't really hurt. But device plug > / -unplug is just as relevant there as in QMP. If it's a show-stopper > for QMP, then I don't think it can be a "minor loss" for HMP. The question is, who do we expect to use HMP at this time? > 2. What if we ever change our minds regarding fine-grained NBD export > control in the human monitor? Add a second command to start the server > without exporting everything? Add a "don't auto-export" argument to the > existing command? The latter. But indeed one alternative, possibly better, is to remove the auto-add and add a "nbd_server_add -a" (for "all") flag to HMP. I think I'll drop the last patch and send a pull request for the others (with your suggested changes), while we discuss this. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 19/10/2012 14:44, Markus Armbruster ha scritto: >> Apropos hotplug. The only way to unexport a block device is to stop the >> NBD server outright. Once the device backend has been exported, >> unplugging the device gets rid of the frontend, but the backend stays >> until you stop the NBD server, or you kill the backend with the big >> drive_del hammer. > > Right. (Though for removable devices you can just eject it, which calls > the close notifier). > >> Makes me wonder whether we need QMP command nbd-server-del. > > Perhaps yes, but it can be added later. Perhaps deleting the backend should automatically unexport it. I suspect that's not the case for the automatic delete on unplug. >> I agree that we can the human monitor is again for human users only, >> thus we don't have to encumber it with the need of tools. I might also >> buy your argument that extra exports don't really hurt. But device plug >> / -unplug is just as relevant there as in QMP. If it's a show-stopper >> for QMP, then I don't think it can be a "minor loss" for HMP. > > The question is, who do we expect to use HMP at this time? Humans. I don't think we have data to justify narrower assumptions. Some human users will happily use libvirt to unplug devices, others may want to do it in the human monitor. "Users of the human monitor don't want to do X" is a rather questionable argument for me. >> 2. What if we ever change our minds regarding fine-grained NBD export >> control in the human monitor? Add a second command to start the server >> without exporting everything? Add a "don't auto-export" argument to the >> existing command? > > The latter. > > But indeed one alternative, possibly better, is to remove the auto-add > and add a "nbd_server_add -a" (for "all") flag to HMP. I think I'll > drop the last patch and send a pull request for the others (with your > suggested changes), while we discuss this. I like the -a idea. In fact, I was pondering it myself :)
Il 19/10/2012 15:53, Markus Armbruster ha scritto: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 19/10/2012 14:44, Markus Armbruster ha scritto: >>> Apropos hotplug. The only way to unexport a block device is to stop the >>> NBD server outright. Once the device backend has been exported, >>> unplugging the device gets rid of the frontend, but the backend stays >>> until you stop the NBD server, or you kill the backend with the big >>> drive_del hammer. >> >> Right. (Though for removable devices you can just eject it, which calls >> the close notifier). >> >>> Makes me wonder whether we need QMP command nbd-server-del. >> >> Perhaps yes, but it can be added later. > > Perhaps deleting the backend should automatically unexport it. I > suspect that's not the case for the automatic delete on unplug. No, because we keep a reference via drive_put_ref. But that's by design, drive_del exists after all and we can add a more fine-grained nbd_server_del too. Paolo
diff --git a/hmp-commands.hx b/hmp-commands.hx index e0b537d..f5d9204 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1274,6 +1274,35 @@ Remove all matches from the access control list, and set the default policy back to @code{deny}. ETEXI + { + .name = "nbd_server_start", + .args_type = "writable:-w,uri:s", + .params = "nbd_server_start [-w] host:port", + .help = "serve block devices on the given host and port", + .mhandler.cmd = hmp_nbd_server_start, + }, +STEXI +@item nbd_server_start @var{host}:@var{port} +@findex nbd_server_start +Start an NBD server on the given host and/or port, and serve all of the +virtual machine's block devices that have an inserted media on it. +The @option{-w} option makes the devices writable. +ETEXI + + { + .name = "nbd_server_stop", + .args_type = "", + .params = "nbd_server_stop", + .help = "stop serving block devices using the NBD protocol", + .mhandler.cmd = hmp_nbd_server_stop, + }, +STEXI +@item nbd_server_stop +@findex nbd_server_stop +Stop the QEMU embedded NBD server. +ETEXI + + #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; + } + } + + 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