[libvirt] [PATCH v4 0/9] Chardev hotplug
The fourth round of my chardev hotplug patches. Michal Privoznik (9): domain_conf: Introduce chardev hotplug helpers qemu: Implement chardev hotplug on config level qemu_monitor_json: Move InetSocketAddress build to a separate function qemu_monitor: Introduce qemuMonitorAttachCharDev qemu_monitor: Introduce qemuMonitorDetachCharDev qemu_command: Honour chardev alias assignment with a function qemu: Introduce qemuBuildChrDeviceStr qemu: Implement chardev hotplug on live level qemuhotplugtest: Introduce test for chardev hotplug src/conf/domain_conf.c | 149 ++++++++++- src/conf/domain_conf.h | 15 ++ src/libvirt_private.syms | 4 + src/qemu/qemu_command.c | 276 +++++++++++++++++---- src/qemu/qemu_command.h | 15 +- src/qemu/qemu_driver.c | 59 ++++- src/qemu/qemu_hotplug.c | 108 ++++++++ src/qemu/qemu_hotplug.h | 6 + src/qemu/qemu_monitor.c | 41 +++ src/qemu/qemu_monitor.h | 5 + src/qemu/qemu_monitor_json.c | 249 +++++++++++++++++-- src/qemu/qemu_monitor_json.h | 5 + tests/qemuhotplugtest.c | 194 ++++++++++++--- .../qemuhotplug-console-virtio.xml | 5 + tests/qemumonitorjsontest.c | 106 ++++++++ .../qemuxml2argv-console-compat-2.xml | 122 +++++++++ 16 files changed, 1243 insertions(+), 116 deletions(-) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml -- 1.8.1.5
For now, only these three helpers are needed: virDomainChrFind - to find a duplicate chardev within VM def virDomainChrInsert - wrapper for inserting a new chardev into VM def virDomainChrRemove - wrapper for removing chardev from VM def There is, however, one internal helper as well: virDomainChrGetDomainPtrs which sets given pointers to one of vmdef->{parallels,serials,consoles,channels} based on passed chardev type. --- src/conf/domain_conf.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 15 ++++++ src/libvirt_private.syms | 4 ++ 3 files changed, 149 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3398d8b..c73ddf3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10004,6 +10004,136 @@ virDomainLeaseRemove(virDomainDefPtr def, return virDomainLeaseRemoveAt(def, i); } +static bool +virDomainChrEquals(virDomainChrDefPtr src, + virDomainChrDefPtr tgt) +{ + if (!src || !tgt) + return src == tgt; + + if (src->deviceType != tgt->deviceType || + !virDomainChrSourceDefIsEqual(&src->source, &tgt->source)) + return false; + + switch ((enum virDomainChrDeviceType) src->deviceType) { + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + if (src->targetType != tgt->targetType) + return false; + switch ((enum virDomainChrChannelTargetType) src->targetType) { + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: + return STREQ_NULLABLE(src->target.name, tgt->target.name); + break; + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: + if (!src->target.addr || !tgt->target.addr) + return src->target.addr == tgt->target.addr; + return memcmp(src->target.addr, tgt->target.addr, + sizeof(*src->target.addr)) == 0; + break; + + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE: + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST: + /* shouldn't happen */ + break; + } + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: + if (src->targetTypeAttr != tgt->targetTypeAttr) + return false; + case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: + return src->target.port == tgt->target.port; + break; + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: + /* shouldn't happen */ + break; + } + return false; +} + +virDomainChrDefPtr +virDomainChrFind(virDomainDefPtr def, + virDomainChrDefPtr target) +{ + virDomainChrDefPtr chr, **arrPtr; + size_t i, *cntPtr; + + virDomainChrGetDomainPtrs(def, target, &arrPtr, &cntPtr); + + for (i = 0; i < *cntPtr; i++) { + chr = (*arrPtr)[i]; + if (virDomainChrEquals(chr, target)) + return chr; + } + return NULL; +} + +void +virDomainChrGetDomainPtrs(virDomainDefPtr vmdef, + virDomainChrDefPtr chr, + virDomainChrDefPtr ***arrPtr, + size_t **cntPtr) +{ + switch ((enum virDomainChrDeviceType) chr->deviceType) { + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: + *arrPtr = &vmdef->parallels; + *cntPtr = &vmdef->nparallels; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: + *arrPtr = &vmdef->serials; + *cntPtr = &vmdef->nserials; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: + *arrPtr = &vmdef->consoles; + *cntPtr = &vmdef->nconsoles; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + *arrPtr = &vmdef->channels; + *cntPtr = &vmdef->nchannels; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: + break; + } +} + +int +virDomainChrInsert(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + + virDomainChrGetDomainPtrs(vmdef, chr, &arrPtr, &cntPtr); + + return VIR_APPEND_ELEMENT(*arrPtr, *cntPtr, chr); +} + +virDomainChrDefPtr +virDomainChrRemove(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + virDomainChrDefPtr ret, **arrPtr; + size_t i, *cntPtr; + + virDomainChrGetDomainPtrs(vmdef, chr, &arrPtr, &cntPtr); + + for (i = 0; i < *cntPtr; i++) { + ret = (*arrPtr)[i]; + + if (virDomainChrEquals(ret, chr)) + break; + } + + if (i == *cntPtr) + return NULL; + + VIR_DELETE_ELEMENT(*arrPtr, i, *cntPtr); + return ret; +} char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index da83eb6..ac4b8c3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2391,6 +2391,21 @@ virDomainLeaseDefPtr virDomainLeaseRemove(virDomainDefPtr def, virDomainLeaseDefPtr lease); +void +virDomainChrGetDomainPtrs(virDomainDefPtr vmdef, + virDomainChrDefPtr chr, + virDomainChrDefPtr ***arrPtr, + size_t **cntPtr); +virDomainChrDefPtr +virDomainChrFind(virDomainDefPtr def, + virDomainChrDefPtr target); +int +virDomainChrInsert(virDomainDefPtr vmdef, + virDomainChrDefPtr chr); +virDomainChrDefPtr +virDomainChrRemove(virDomainDefPtr vmdef, + virDomainChrDefPtr chr); + int virDomainSaveXML(const char *configDir, virDomainDefPtr def, const char *xml); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 59583ec..7fb32b8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -114,6 +114,10 @@ virDomainChrConsoleTargetTypeToString; virDomainChrDefForeach; virDomainChrDefFree; virDomainChrDefNew; +virDomainChrFind; +virDomainChrGetDomainPtrs; +virDomainChrInsert; +virDomainChrRemove; virDomainChrSerialTargetTypeFromString; virDomainChrSerialTargetTypeToString; virDomainChrSourceDefCopy; -- 1.8.1.5
On Wed, Jul 10, 2013 at 07:02:51PM +0200, Michal Privoznik wrote:
For now, only these three helpers are needed: virDomainChrFind - to find a duplicate chardev within VM def virDomainChrInsert - wrapper for inserting a new chardev into VM def virDomainChrRemove - wrapper for removing chardev from VM def
There is, however, one internal helper as well: virDomainChrGetDomainPtrs which sets given pointers to one of vmdef->{parallels,serials,consoles,channels} based on passed chardev type. --- src/conf/domain_conf.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 15 ++++++ src/libvirt_private.syms | 4 ++ 3 files changed, 149 insertions(+)
ACK, remember, no need to re-post patches which were acked in a previous posting if they don't depend on anything. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
There are two levels on which a device may be hotplugged: config and live. The config level requires just an insert or remove from internal domain definition structure, which is exactly what this patch does. There is currently no implementation for a chardev update action, as there's not much to be updated. But more importantly, the only thing that can be updated is path or socket address by which chardevs are distinguished. So the update action is currently not supported. --- src/conf/domain_conf.c | 19 +++++++++++++++++-- src/qemu/qemu_driver.c | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c73ddf3..2370555 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1756,10 +1756,12 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) case VIR_DOMAIN_DEVICE_RNG: virDomainRNGDefFree(def->data.rng); break; + case VIR_DOMAIN_DEVICE_CHR: + virDomainChrDefFree(def->data.chr); + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_SMARTCARD: - case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_LAST: @@ -9385,6 +9387,17 @@ virDomainDeviceDefParse(const char *xmlStr, dev->type = VIR_DOMAIN_DEVICE_RNG; if (!(dev->data.rng = virDomainRNGDefParseXML(node, ctxt, flags))) goto error; + } else if (xmlStrEqual(node->name, BAD_CAST "channel") || + xmlStrEqual(node->name, BAD_CAST "console") || + xmlStrEqual(node->name, BAD_CAST "parallel") || + xmlStrEqual(node->name, BAD_CAST "serial")) { + dev->type = VIR_DOMAIN_DEVICE_CHR; + if (!(dev->data.chr = virDomainChrDefParseXML(ctxt, + node, + def->seclabels, + def->nseclabels, + flags))) + goto error; } else { virReportError(VIR_ERR_XML_ERROR, "%s", _("unknown device type")); goto error; @@ -17932,9 +17945,11 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, case VIR_DOMAIN_DEVICE_RNG: rc = virDomainRNGDefFormat(&buf, src->data.rng, flags); break; + case VIR_DOMAIN_DEVICE_CHR: + rc = virDomainChrDefFormat(&buf, src->data.chr, flags); + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: - case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_LAST: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b0180c9..d858131 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6607,6 +6607,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, virDomainHostdevDefPtr hostdev; virDomainLeaseDefPtr lease; virDomainControllerDefPtr controller; + virDomainChrDefPtr chr; switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -6682,10 +6683,23 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, return -1; break; + case VIR_DOMAIN_DEVICE_CHR: + chr = dev->data.chr; + if (virDomainChrFind(vmdef, chr)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("chardev already exists")); + return -1; + } + + if (virDomainChrInsert(vmdef, chr) < 0) + return -1; + dev->data.chr = NULL; + break; + default: - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("persistent attach of device is not supported")); - return -1; + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("persistent attach of device is not supported")); + return -1; } return 0; } @@ -6700,6 +6714,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainHostdevDefPtr hostdev, det_hostdev; virDomainLeaseDefPtr lease, det_lease; virDomainControllerDefPtr cont, det_cont; + virDomainChrDefPtr chr; int idx; char mac[VIR_MAC_STRING_BUFLEN]; @@ -6767,6 +6782,18 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, break; + case VIR_DOMAIN_DEVICE_CHR: + if (!(chr = virDomainChrRemove(vmdef, dev->data.chr))) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("device not present in domain configuration")); + return -1; + } + + virDomainChrDefFree(chr); + virDomainChrDefFree(dev->data.chr); + dev->data.chr = NULL; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("persistent detach of device is not supported")); -- 1.8.1.5
On 07/10/2013 01:02 PM, Michal Privoznik wrote:
There are two levels on which a device may be hotplugged: config and live. The config level requires just an insert or remove from internal domain definition structure, which is exactly what this patch does. There is currently no implementation for a chardev update action, as there's not much to be updated. But more importantly, the only thing that can be updated is path or socket address by which chardevs are distinguished. So the update action is currently not supported.
Will this help address https://bugzilla.redhat.com/show_bug.cgi?id=982304 Maybe I should move or clone that to Fedora instead of RHEL? -- Chris Evich, RHCA, RHCE, RHCDS, RHCSS Quality Assurance Engineer e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214
On 10.07.2013 20:42, Chris Evich wrote:
On 07/10/2013 01:02 PM, Michal Privoznik wrote:
There are two levels on which a device may be hotplugged: config and live. The config level requires just an insert or remove from internal domain definition structure, which is exactly what this patch does. There is currently no implementation for a chardev update action, as there's not much to be updated. But more importantly, the only thing that can be updated is path or socket address by which chardevs are distinguished. So the update action is currently not supported.
Will this help address https://bugzilla.redhat.com/show_bug.cgi?id=982304
Maybe I should move or clone that to Fedora instead of RHEL?
For a very specific case of hotplugging character devices - yes. For general devices which can't be hotplugged yet - no. This patch(set) in fact just sizes up the set of devices that can be hotplugged. Michal
On Wed, Jul 10, 2013 at 07:02:52PM +0200, Michal Privoznik wrote:
There are two levels on which a device may be hotplugged: config and live. The config level requires just an insert or remove from internal domain definition structure, which is exactly what this patch does. There is currently no implementation for a chardev update action, as there's not much to be updated. But more importantly, the only thing that can be updated is path or socket address by which chardevs are distinguished. So the update action is currently not supported.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b0180c9..d858131 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6607,6 +6607,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, virDomainHostdevDefPtr hostdev; virDomainLeaseDefPtr lease; virDomainControllerDefPtr controller; + virDomainChrDefPtr chr;
switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -6682,10 +6683,23 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, return -1; break;
+ case VIR_DOMAIN_DEVICE_CHR: + chr = dev->data.chr; + if (virDomainChrFind(vmdef, chr)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("chardev already exists")); + return -1; + } + + if (virDomainChrInsert(vmdef, chr) < 0) + return -1; + dev->data.chr = NULL; + break;
Hmm, this is unconditionally adding the device to the list, which in general is fine..... except for the magic serial/console duplication. If we have zero serial devices + zero console devices, and then hotplug a serial device, we must duplicate that as the first console device. We should also refuse to allow you to hotplug a <console> element with target type == serial.
@@ -6767,6 +6782,18 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
break;
+ case VIR_DOMAIN_DEVICE_CHR: + if (!(chr = virDomainChrRemove(vmdef, dev->data.chr))) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("device not present in domain configuration")); + return -1; + } + + virDomainChrDefFree(chr); + virDomainChrDefFree(dev->data.chr); + dev->data.chr = NULL; + break;
And if we remove the first serial device here, then we also need to remove the compat console device with targettype==serial And probably ought to forbid removing a <console> with type=serial, instead requiring them to remove the <serial> device instead. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Currently, we are building InetSocketAddress qemu json type within the qemuMonitorJSONNBDServerStart function. However, other future functions may profit from the code as well. So it should be moved into a static function. --- src/qemu/qemu_monitor_json.c | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d326551..626db8b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4738,6 +4738,31 @@ cleanup: return ret; } +static virJSONValuePtr +qemuMonitorJSONBuildInetSocketAddress(const char *host, + const char *port) +{ + virJSONValuePtr addr = NULL; + virJSONValuePtr data = NULL; + + if (!(data = virJSONValueNewObject()) || + !(addr = virJSONValueNewObject())) + goto error; + + /* port is really expected as a string here by qemu */ + if (virJSONValueObjectAppendString(data, "host", host) < 0 || + virJSONValueObjectAppendString(data, "port", port) < 0 || + virJSONValueObjectAppendString(addr, "type", "inet") < 0 || + virJSONValueObjectAppend(addr, "data", data) < 0) + goto error; + + return addr; +error: + virJSONValueFree(data); + virJSONValueFree(addr); + return NULL; +} + int qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon, const char *host, @@ -4746,24 +4771,14 @@ qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon, int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; - virJSONValuePtr data = NULL; virJSONValuePtr addr = NULL; char *port_str = NULL; - if (!(data = virJSONValueNewObject()) || - !(addr = virJSONValueNewObject()) || - (virAsprintf(&port_str, "%u", port) < 0)) - goto cleanup; - - /* port is really expected as a string here by qemu */ - if (virJSONValueObjectAppendString(data, "host", host) < 0 || - virJSONValueObjectAppendString(data, "port", port_str) < 0 || - virJSONValueObjectAppendString(addr, "type", "inet") < 0 || - virJSONValueObjectAppend(addr, "data", data) < 0) - goto cleanup; + if (virAsprintf(&port_str, "%u", port) < 0) + return ret; - /* From now on, @data is part of @addr */ - data = NULL; + if (!(addr = qemuMonitorJSONBuildInetSocketAddress(host, port_str))) + return ret; if (!(cmd = qemuMonitorJSONMakeCommand("nbd-server-start", "a:addr", addr, @@ -4786,7 +4801,6 @@ cleanup: virJSONValueFree(reply); virJSONValueFree(cmd); virJSONValueFree(addr); - virJSONValueFree(data); return ret; } -- 1.8.1.5
On Wed, Jul 10, 2013 at 07:02:53PM +0200, Michal Privoznik wrote:
Currently, we are building InetSocketAddress qemu json type within the qemuMonitorJSONNBDServerStart function. However, other future functions may profit from the code as well. So it should be moved into a static function. --- src/qemu/qemu_monitor_json.c | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The function being introduced is responsible for preparing and executing 'chardev-add' qemu monitor command. Moreover, in case of PTY chardev, the corresponding pty path is updated. --- src/qemu/qemu_monitor.c | 21 +++++ src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 182 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + tests/qemumonitorjsontest.c | 81 +++++++++++++++++++ 5 files changed, 290 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 067b368..c85f826 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3621,3 +3621,24 @@ int qemuMonitorGetTPMTypes(qemuMonitorPtr mon, return qemuMonitorJSONGetTPMTypes(mon, tpmtypes); } + +int qemuMonitorAttachCharDev(qemuMonitorPtr mon, + const char *chrID, + virDomainChrSourceDefPtr chr) +{ + VIR_DEBUG("mon=%p chrID=%s chr=%p", mon, chrID, chr); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("JSON monitor is required")); + return -1; + } + + return qemuMonitorJSONAttachCharDev(mon, chrID, chr); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 78011ee..734e09b 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -701,6 +701,9 @@ int qemuMonitorGetTPMModels(qemuMonitorPtr mon, int qemuMonitorGetTPMTypes(qemuMonitorPtr mon, char ***tpmtypes); +int qemuMonitorAttachCharDev(qemuMonitorPtr mon, + const char *chrID, + virDomainChrSourceDefPtr chr); /** * When running two dd process and using <> redirection, we need a * shell that will not truncate files. These two strings serve that diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 626db8b..8e5a67c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4763,6 +4763,29 @@ error: return NULL; } +static virJSONValuePtr +qemuMonitorJSONBuildUnixSocketAddress(const char *path) +{ + virJSONValuePtr addr = NULL; + virJSONValuePtr data = NULL; + + if (!(data = virJSONValueNewObject()) || + !(addr = virJSONValueNewObject())) + goto error; + + if (virJSONValueObjectAppendString(data, "path", path) < 0 || + virJSONValueObjectAppendString(addr, "type", "unix") < 0 || + virJSONValueObjectAppend(addr, "data", data) < 0) + goto error; + + return addr; +error: + virReportOOMError(); + virJSONValueFree(data); + virJSONValueFree(addr); + return NULL; +} + int qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon, const char *host, @@ -4938,3 +4961,162 @@ int qemuMonitorJSONGetTPMTypes(qemuMonitorPtr mon, { return qemuMonitorJSONGetStringArray(mon, "query-tpm-types", tpmtypes); } + +static virJSONValuePtr +qemuMonitorJSONAttachCharDevCommand(const char *chrID, + const virDomainChrSourceDefPtr chr) +{ + virJSONValuePtr ret; + virJSONValuePtr backend; + virJSONValuePtr data = NULL; + virJSONValuePtr addr = NULL; + const char *backend_type = NULL; + bool telnet; + + if (!(backend = virJSONValueNewObject()) || + !(data = virJSONValueNewObject())) { + goto no_memory; + } + + switch ((enum virDomainChrType) chr->type) { + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + backend_type = "null"; + break; + + case VIR_DOMAIN_CHR_TYPE_PTY: + backend_type = "pty"; + break; + + case VIR_DOMAIN_CHR_TYPE_FILE: + backend_type = "file"; + if (virJSONValueObjectAppendString(data, "out", chr->data.file.path) < 0) + goto no_memory; + break; + + case VIR_DOMAIN_CHR_TYPE_DEV: + backend_type = STRPREFIX(chrID, "parallel") ? "parallel" : "serial"; + if (virJSONValueObjectAppendString(data, "device", + chr->data.file.path) < 0) + goto no_memory; + break; + + case VIR_DOMAIN_CHR_TYPE_TCP: + backend_type = "socket"; + addr = qemuMonitorJSONBuildInetSocketAddress(chr->data.tcp.host, + chr->data.tcp.service); + if (!addr || + virJSONValueObjectAppend(data, "addr", addr) < 0) + goto no_memory; + addr = NULL; + + telnet = chr->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET; + + if (virJSONValueObjectAppendBoolean(data, "wait", false) < 0 || + virJSONValueObjectAppendBoolean(data, "telnet", telnet) < 0 || + virJSONValueObjectAppendBoolean(data, "server", chr->data.tcp.listen) < 0) + goto no_memory; + break; + + case VIR_DOMAIN_CHR_TYPE_UDP: + backend_type = "socket"; + addr = qemuMonitorJSONBuildInetSocketAddress(chr->data.udp.connectHost, + chr->data.udp.connectService); + if (!addr || + virJSONValueObjectAppend(data, "addr", addr) < 0) + goto no_memory; + addr = NULL; + break; + + case VIR_DOMAIN_CHR_TYPE_UNIX: + backend_type = "socket"; + addr = qemuMonitorJSONBuildUnixSocketAddress(chr->data.nix.path); + + if (!addr || + virJSONValueObjectAppend(data, "addr", addr) < 0) + goto no_memory; + addr = NULL; + + if (virJSONValueObjectAppendBoolean(data, "wait", false) < 0 || + virJSONValueObjectAppendBoolean(data, "server", chr->data.nix.listen) < 0) + goto no_memory; + break; + + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + case VIR_DOMAIN_CHR_TYPE_PIPE: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_LAST: + virReportError(VIR_ERR_OPERATION_FAILED, + _("Unsupported char device type '%d'"), + chr->type); + goto error; + } + + if (virJSONValueObjectAppendString(backend, "type", backend_type) < 0 || + virJSONValueObjectAppend(backend, "data", data) < 0) + goto no_memory; + data = NULL; + + if (!(ret = qemuMonitorJSONMakeCommand("chardev-add", + "s:id", chrID, + "a:backend", backend, + NULL))) + goto error; + + return ret; + +no_memory: + virReportOOMError(); +error: + virJSONValueFree(addr); + virJSONValueFree(data); + virJSONValueFree(backend); + return NULL; +} + + +int +qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon, + const char *chrID, + virDomainChrSourceDefPtr chr) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + if (!(cmd = qemuMonitorJSONAttachCharDevCommand(chrID, chr))) + return ret; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + if (chr->type == VIR_DOMAIN_CHR_TYPE_PTY) { + virJSONValuePtr data; + const char *path; + + if (!(data = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("chardev-add reply was missing return data")); + goto cleanup; + } + + if (!(path = virJSONValueObjectGetString(data, "pty"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("chardev-add reply was missing pty path")); + goto cleanup; + } + + if (VIR_STRDUP(chr->data.file.path, path) < 0) + goto cleanup; + } + + ret = 0; + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d79b86b..e0a4883 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -353,4 +353,7 @@ int qemuMonitorJSONGetTPMTypes(qemuMonitorPtr mon, char ***tpmtypes) ATTRIBUTE_NONNULL(2); +int qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon, + const char *chrID, + virDomainChrSourceDefPtr chr); #endif /* QEMU_MONITOR_JSON_H */ diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index acc94ca..411a7a7 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -594,6 +594,86 @@ cleanup: return ret; } +static int +testQemuMonitorJSONAttachChardev(const void *data) +{ + const virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNew(true, xmlopt); + virDomainChrSourceDef chr; + int ret = 0; + + if (!test) + return -1; + +#define DO_CHECK(chrID, reply, fail) \ + if (qemuMonitorTestAddItem(test, "chardev-add", reply) < 0) \ + goto cleanup; \ + if (qemuMonitorAttachCharDev(qemuMonitorTestGetMonitor(test), \ + chrID, &chr) < 0) \ + ret = fail ? ret : -1; \ + else \ + ret = fail ? -1 : ret; \ + +#define CHECK(chrID, reply) \ + DO_CHECK(chrID, reply, false) + +#define CHECK_FAIL(chrID, reply) \ + DO_CHECK(chrID, reply, true) + + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_NULL }; + CHECK("chr_null", "{\"return\": {}}"); + + chr = (virDomainChrSourceDef) { .type =VIR_DOMAIN_CHR_TYPE_VC }; + CHECK("chr_vc", "{\"return\": {}}"); + +#define PTY_PATH "/dev/ttyS0" + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; + CHECK("chr_pty", "{\"return\": {\"pty\" : \"" PTY_PATH "\"}}"); + if (STRNEQ_NULLABLE(PTY_PATH, chr.data.file.path)) { + VIR_FREE(chr.data.file.path); + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected PTY path: %s got: %s", + PTY_PATH, NULLSTR(chr.data.file.path)); + ret = -1; + } + VIR_FREE(chr.data.file.path); + + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PTY }; + CHECK_FAIL("chr_pty_fail", "{\"return\": {}}"); +#undef PTY_PATH + + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_FILE }; + CHECK("chr_file", "{\"return\": {}}"); + + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_DEV }; + CHECK("chr_dev", "{\"return\": {}}"); + + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_TCP }; + CHECK("chr_tcp", "{\"return\": {}}"); + + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_UDP }; + CHECK("chr_udp", "{\"return\": {}}"); + + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_UNIX }; + CHECK("chr_unix", "{\"return\": {}}"); + + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_SPICEVMC }; + CHECK_FAIL("chr_spicevmc", "{\"return\": {}}"); + + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PIPE }; + CHECK_FAIL("chr_pipe", "{\"return\": {}}"); + + chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_STDIO }; + CHECK_FAIL("chr_stdio", "{\"return\": {}}"); + +#undef CHECK +#undef CHECK_FAIL +#undef DO_CHECK + +cleanup: + qemuMonitorTestFree(test); + return ret; +} static int mymain(void) @@ -623,6 +703,7 @@ mymain(void) DO_TEST(GetCommands); DO_TEST(GetTPMModels); DO_TEST(GetCommandLineOptionParameters); + DO_TEST(AttachChardev); virObjectUnref(xmlopt); -- 1.8.1.5
On Wed, Jul 10, 2013 at 07:02:54PM +0200, Michal Privoznik wrote:
The function being introduced is responsible for preparing and executing 'chardev-add' qemu monitor command. Moreover, in case of PTY chardev, the corresponding pty path is updated. --- src/qemu/qemu_monitor.c | 21 +++++ src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 182 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + tests/qemumonitorjsontest.c | 81 +++++++++++++++++++ 5 files changed, 290 insertions(+)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This function wraps 'chardev-remove' qemu monitor command around. It takes chardev alias as its single argument besides qemu monitor pointer. --- src/qemu/qemu_monitor.c | 20 ++++++++++++++++++++ src/qemu/qemu_monitor.h | 2 ++ src/qemu/qemu_monitor_json.c | 23 +++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 2 ++ tests/qemumonitorjsontest.c | 25 +++++++++++++++++++++++++ 5 files changed, 72 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c85f826..23a19ae 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3642,3 +3642,23 @@ int qemuMonitorAttachCharDev(qemuMonitorPtr mon, return qemuMonitorJSONAttachCharDev(mon, chrID, chr); } + +int qemuMonitorDetachCharDev(qemuMonitorPtr mon, + const char *chrID) +{ + VIR_DEBUG("mon=%p chrID=%s", mon, chrID); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("JSON monitor is required")); + return -1; + } + + return qemuMonitorJSONDetachCharDev(mon, chrID); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 734e09b..4d83bef 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -704,6 +704,8 @@ int qemuMonitorGetTPMTypes(qemuMonitorPtr mon, int qemuMonitorAttachCharDev(qemuMonitorPtr mon, const char *chrID, virDomainChrSourceDefPtr chr); +int qemuMonitorDetachCharDev(qemuMonitorPtr mon, + const char *chrID); /** * When running two dd process and using <> redirection, we need a * shell that will not truncate files. These two strings serve that diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8e5a67c..e5fdacd 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5120,3 +5120,26 @@ cleanup: virJSONValueFree(reply); return ret; } + +int +qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon, + const char *chrID) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("chardev-remove", + "s:id", chrID, + NULL))) + return ret; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index e0a4883..2660773 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -356,4 +356,6 @@ int qemuMonitorJSONGetTPMTypes(qemuMonitorPtr mon, int qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon, const char *chrID, virDomainChrSourceDefPtr chr); +int qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon, + const char *chrID); #endif /* QEMU_MONITOR_JSON_H */ diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 411a7a7..b30e16d 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -676,6 +676,30 @@ cleanup: } static int +testQemuMonitorJSONDetachChardev(const void *data) +{ + const virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNew(true, xmlopt); + int ret = -1; + + if (!test) + return ret; + + if (qemuMonitorTestAddItem(test, "chardev-remove", "{\"return\": {}}") < 0) + goto cleanup; + + if (qemuMonitorDetachCharDev(qemuMonitorTestGetMonitor(test), + "dummy_chrID") < 0) + goto cleanup; + + ret = 0; + +cleanup: + qemuMonitorTestFree(test); + return ret; +} + +static int mymain(void) { int ret = 0; @@ -704,6 +728,7 @@ mymain(void) DO_TEST(GetTPMModels); DO_TEST(GetCommandLineOptionParameters); DO_TEST(AttachChardev); + DO_TEST(DetachChardev); virObjectUnref(xmlopt); -- 1.8.1.5
On Wed, Jul 10, 2013 at 07:02:55PM +0200, Michal Privoznik wrote:
This function wraps 'chardev-remove' qemu monitor command around. It takes chardev alias as its single argument besides qemu monitor pointer. --- src/qemu/qemu_monitor.c | 20 ++++++++++++++++++++ src/qemu/qemu_monitor.h | 2 ++ src/qemu/qemu_monitor_json.c | 23 +++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 2 ++ tests/qemumonitorjsontest.c | 25 +++++++++++++++++++++++++ 5 files changed, 72 insertions(+)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The chardev alias assignment is going to be needed in a separate places, so it should be moved into a separate function rather than copying code randomly around. --- src/qemu/qemu_command.c | 38 ++++++++++++++++++++++++++++++++++---- src/qemu/qemu_command.h | 3 +++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 325ef38..6cf46a2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -863,6 +863,36 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx); } +int +qemuAssignDeviceChrAlias(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrDefPtr chr, + ssize_t idx) +{ + const char *prefix = NULL; + + switch ((enum virDomainChrDeviceType) chr->deviceType) { + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: + prefix = "parallel"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: + prefix = "serial"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: + prefix = "console"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + prefix = "channel"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: + return -1; + } + + return virAsprintf(&chr->info.alias, "%s%zd", prefix, idx); +} int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) @@ -918,19 +948,19 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nparallels; i++) { - if (virAsprintf(&def->parallels[i]->info.alias, "parallel%d", i) < 0) + if (qemuAssignDeviceChrAlias(def, def->parallels[i], i) < 0) return -1; } for (i = 0; i < def->nserials; i++) { - if (virAsprintf(&def->serials[i]->info.alias, "serial%d", i) < 0) + if (qemuAssignDeviceChrAlias(def, def->serials[i], i) < 0) return -1; } for (i = 0; i < def->nchannels; i++) { - if (virAsprintf(&def->channels[i]->info.alias, "channel%d", i) < 0) + if (qemuAssignDeviceChrAlias(def, def->channels[i], i) < 0) return -1; } for (i = 0; i < def->nconsoles; i++) { - if (virAsprintf(&def->consoles[i]->info.alias, "console%d", i) < 0) + if (qemuAssignDeviceChrAlias(def, def->consoles[i], i) < 0) return -1; } for (i = 0; i < def->nhubs; i++) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2993448..e92c78a 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -260,6 +260,9 @@ int qemuAssignDeviceDiskAlias(virDomainDefPtr vmdef, int qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev, int idx); int qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller); int qemuAssignDeviceRedirdevAlias(virDomainDefPtr def, virDomainRedirdevDefPtr redirdev, int idx); +int qemuAssignDeviceChrAlias(virDomainDefPtr def, + virDomainChrDefPtr chr, + ssize_t idx); int qemuParseKeywords(const char *str, -- 1.8.1.5
On Wed, Jul 10, 2013 at 07:02:56PM +0200, Michal Privoznik wrote:
The chardev alias assignment is going to be needed in a separate places, so it should be moved into a separate function rather than copying code randomly around. --- src/qemu/qemu_command.c | 38 ++++++++++++++++++++++++++++++++++---- src/qemu/qemu_command.h | 3 +++ 2 files changed, 37 insertions(+), 4 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The function being introduced is responsible for creating command line argument for '-device' for given character device. Based on the chardev type, it calls appropriate qemuBuild.*ChrDeviceStr(), e.g. qemuBuildSerialChrDeviceStr() for serial chardev and so on. --- src/qemu/qemu_command.c | 202 +++++++++++++++++++++++++++++++++++++----------- src/qemu/qemu_command.h | 12 +-- 2 files changed, 163 insertions(+), 51 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6cf46a2..063d76b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6593,6 +6593,21 @@ cleanup: return ret; } +static int +qemuBuildChrDeviceCommandLine(virCommandPtr cmd, + virDomainDefPtr def, + virDomainChrDefPtr chr, + virQEMUCapsPtr qemuCaps) +{ + char *devstr = NULL; + + if (qemuBuildChrDeviceStr(&devstr, def, chr, qemuCaps) < 0) + return -1; + + virCommandAddArgList(cmd, "-device", devstr, NULL); + return 0; +} + qemuBuildCommandLineCallbacks buildCommandLineCallbacks = { .qemuGetSCSIDeviceSgName = virSCSIDeviceGetSgName, }; @@ -7670,13 +7685,8 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr); - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildChrDeviceStr(serial, qemuCaps, - def->os.arch, - def->os.machine))) + if (qemuBuildChrDeviceCommandLine(cmd, def, serial, qemuCaps) < 0) goto error; - virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); } else { virCommandAddArg(cmd, "-serial"); if (!(devstr = qemuBuildChrArgStr(&serial->source, NULL))) @@ -7707,10 +7717,8 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr); - virCommandAddArg(cmd, "-device"); - virCommandAddArgFormat(cmd, "isa-parallel,chardev=char%s,id=%s", - parallel->info.alias, - parallel->info.alias); + if (qemuBuildChrDeviceCommandLine(cmd, def, parallel, qemuCaps) < 0) + goto error; } else { virCommandAddArg(cmd, "-parallel"); if (!(devstr = qemuBuildChrArgStr(¶llel->source, NULL))) @@ -7724,8 +7732,6 @@ qemuBuildCommandLine(virConnectPtr conn, for (i = 0; i < def->nchannels; i++) { virDomainChrDefPtr channel = def->channels[i]; char *devstr; - char *addr; - int port; switch (channel->targetType) { case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: @@ -7744,17 +7750,10 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr); - addr = virSocketAddrFormat(channel->target.addr); - if (!addr) + if (qemuBuildChrDeviceStr(&devstr, def, channel, qemuCaps) < 0) goto error; - port = virSocketAddrGetPort(channel->target.addr); - - virCommandAddArg(cmd, "-netdev"); - virCommandAddArgFormat(cmd, - "user,guestfwd=tcp:%s:%i,chardev=char%s,id=user-%s", - addr, port, channel->info.alias, - channel->info.alias); - VIR_FREE(addr); + virCommandAddArgList(cmd, "-netdev", devstr, NULL); + VIR_FREE(devstr); break; case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: @@ -7780,12 +7779,8 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(devstr); } - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildVirtioSerialPortDevStr(channel, - qemuCaps))) + if (qemuBuildChrDeviceCommandLine(cmd, def, channel, qemuCaps) < 0) goto error; - virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); break; } } @@ -7817,11 +7812,8 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr); - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildSclpDevStr(console))) + if (qemuBuildChrDeviceCommandLine(cmd, def, console, qemuCaps) < 0) goto error; - virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); break; case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO: @@ -7839,12 +7831,8 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr); - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildVirtioSerialPortDevStr(console, - qemuCaps))) + if (qemuBuildChrDeviceCommandLine(cmd, def, console, qemuCaps) < 0) goto error; - virCommandAddArg(cmd, devstr); - VIR_FREE(devstr); break; case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL: @@ -8534,11 +8522,12 @@ error: /* This function generates the correct '-device' string for character * devices of each architecture. */ -char * -qemuBuildChrDeviceStr(virDomainChrDefPtr serial, - virQEMUCapsPtr qemuCaps, - virArch arch, - char *machine) +static int +qemuBuildSerialChrDeviceStr(char **deviceStr, + virDomainChrDefPtr serial, + virQEMUCapsPtr qemuCaps, + virArch arch, + char *machine) { virBuffer cmd = VIR_BUFFER_INITIALIZER; @@ -8570,7 +8559,7 @@ qemuBuildChrDeviceStr(virDomainChrDefPtr serial, } if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) - goto error; + goto error; } } @@ -8579,13 +8568,136 @@ qemuBuildChrDeviceStr(virDomainChrDefPtr serial, goto error; } - return virBufferContentAndReset(&cmd); + *deviceStr = virBufferContentAndReset(&cmd); + return 0; - error: +error: virBufferFreeAndReset(&cmd); - return NULL; + return -1; +} + +static int +qemuBuildParallelChrDeviceStr(char **deviceStr, + virDomainChrDefPtr chr) +{ + if (virAsprintf(deviceStr, "isa-parallel,chardev=char%s,id=%s", + chr->info.alias, chr->info.alias) < 0) { + virReportOOMError(); + return -1; + } + return 0; } +static int +qemuBuildChannelChrDeviceStr(char **deviceStr, + virDomainChrDefPtr chr, + virQEMUCapsPtr qemuCaps) +{ + int ret = -1; + char *addr = NULL; + int port; + + switch ((enum virDomainChrChannelTargetType) chr->targetType) { + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: + + addr = virSocketAddrFormat(chr->target.addr); + if (!addr) + return ret; + port = virSocketAddrGetPort(chr->target.addr); + + if (virAsprintf(deviceStr, + "user,guestfwd=tcp:%s:%i,chardev=char%s,id=user-%s", + addr, port, chr->info.alias, chr->info.alias) < 0) { + virReportOOMError(); + goto cleanup; + } + break; + + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: + if (!(*deviceStr = qemuBuildVirtioSerialPortDevStr(chr, qemuCaps))) + goto cleanup; + break; + + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE: + case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST: + return ret; + } + + ret = 0; +cleanup: + VIR_FREE(addr); + return ret; +} + +static int +qemuBuildConsoleChrDeviceStr(char **deviceStr, + virDomainChrDefPtr chr, + virQEMUCapsPtr qemuCaps) +{ + int ret = -1; + + switch ((enum virDomainChrConsoleTargetType) chr->targetType) { + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLP: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLPLM: + if (!(*deviceStr = qemuBuildSclpDevStr(chr))) + goto cleanup; + break; + + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO: + if (!(*deviceStr = qemuBuildVirtioSerialPortDevStr(chr, qemuCaps))) + goto cleanup; + break; + + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ: + case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST: + break; + } + + ret = 0; +cleanup: + return ret; +} + +int +qemuBuildChrDeviceStr(char **deviceStr, + virDomainDefPtr vmdef, + virDomainChrDefPtr chr, + virQEMUCapsPtr qemuCaps) +{ + int ret = -1; + + switch ((enum virDomainChrDeviceType) chr->deviceType) { + case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: + ret = qemuBuildSerialChrDeviceStr(deviceStr, chr, qemuCaps, + vmdef->os.arch, + vmdef->os.machine); + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: + ret = qemuBuildParallelChrDeviceStr(deviceStr, chr); + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + ret = qemuBuildChannelChrDeviceStr(deviceStr, chr, qemuCaps); + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: + ret = qemuBuildConsoleChrDeviceStr(deviceStr, chr, qemuCaps); + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: + return ret; + } + + return ret; +} + + /* * This method takes a string representing a QEMU command line ARGV set * optionally prefixed by a list of environment variables. It then tries diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index e92c78a..88d7099 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -75,12 +75,12 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, qemuBuildCommandLineCallbacksPtr callbacks) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(11); -/* Generate string for arch-specific '-device' parameter */ -char * -qemuBuildChrDeviceStr (virDomainChrDefPtr serial, - virQEMUCapsPtr qemuCaps, - virArch arch, - char *machine); +/* Generate '-device' string for chardev device */ +int +qemuBuildChrDeviceStr(char **deviceStr, + virDomainDefPtr vmdef, + virDomainChrDefPtr chr, + virQEMUCapsPtr qemuCaps); /* With vlan == -1, use netdev syntax, else old hostnet */ char * qemuBuildHostNetStr(virDomainNetDefPtr net, -- 1.8.1.5
On Wed, Jul 10, 2013 at 07:02:57PM +0200, Michal Privoznik wrote:
The function being introduced is responsible for creating command line argument for '-device' for given character device. Based on the chardev type, it calls appropriate qemuBuild.*ChrDeviceStr(), e.g. qemuBuildSerialChrDeviceStr() for serial chardev and so on. --- src/qemu/qemu_command.c | 202 +++++++++++++++++++++++++++++++++++++----------- src/qemu/qemu_command.h | 12 +-- 2 files changed, 163 insertions(+), 51 deletions(-)
ACK, I'm assuming we have test coverage already that is validating this code. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/10/2013 01:02 PM, Michal Privoznik wrote:
The function being introduced is responsible for creating command line argument for '-device' for given character device. Based on the chardev type, it calls appropriate qemuBuild.*ChrDeviceStr(), e.g. qemuBuildSerialChrDeviceStr() for serial chardev and so on. --- src/qemu/qemu_command.c | 202 +++++++++++++++++++++++++++++++++++++----------- src/qemu/qemu_command.h | 12 +-- 2 files changed, 163 insertions(+), 51 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6cf46a2..063d76b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6593,6 +6593,21 @@ cleanup: return ret; }
+static int +qemuBuildChrDeviceCommandLine(virCommandPtr cmd, + virDomainDefPtr def, + virDomainChrDefPtr chr, + virQEMUCapsPtr qemuCaps) +{ + char *devstr = NULL; + + if (qemuBuildChrDeviceStr(&devstr, def, chr, qemuCaps) < 0) + return -1; + + virCommandAddArgList(cmd, "-device", devstr, NULL);
"make -C tests valgrind" reports: ==16114== 1,100 bytes in 1 blocks are definitely lost in loss record 100 of 118 ==16114== at 0x4A0887C: malloc (vg_replace_malloc.c:270) ==16114== by 0x4A089F0: realloc (vg_replace_malloc.c:662) ==16114== by 0x4C70BE7: virReallocN (viralloc.c:233) ==16114== by 0x4C73BA0: virBufferGrow (virbuffer.c:129) ==16114== by 0x4C740AE: virBufferVasprintf (virbuffer.c:321) ==16114== by 0x4C74223: virBufferAsprintf (virbuffer.c:294) ==16114== by 0x437DA2: qemuBuildChrDeviceStr (qemu_command.c:8547) ==16114== by 0x438027: qemuBuildChrDeviceCommandLine (qemu_command.c:6607) ==16114== by 0x43BAEF: qemuBuildCommandLine (qemu_command.c:7693) ==16114== by 0x4247EC: testCompareXMLToArgvHelper (qemuxml2argvtest.c:157) ==16114== by 0x425E97: virtTestRun (testutils.c:159) ==16114== by 0x4205F1: mymain (qemuxml2argvtest.c:715) ==16114== You need a VIR_FREE(devstr);
+ return 0; +} +
Since previous patches has prepared everything for us, we may now implement live hotplug of a character device. --- src/qemu/qemu_command.c | 38 ++++++++++++++++- src/qemu/qemu_driver.c | 26 ++++++++++-- src/qemu/qemu_hotplug.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 6 +++ 4 files changed, 173 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 063d76b..6842cab 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -863,8 +863,41 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx); } +static ssize_t +qemuGetNextChrDevIndex(virDomainDefPtr def, + virDomainChrDefPtr chr, + const char *prefix) +{ + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + size_t i; + ssize_t idx = 0; + const char *prefix2 = NULL; + + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE) + prefix2 = "serial"; + + virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr); + + for (i = 0; i < *cntPtr; i++) { + ssize_t thisidx; + if (((thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix)) < 0) && + (prefix2 && + (thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix2)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to determine device index for character device")); + return -1; + } + if (thisidx >= idx) + idx = thisidx + 1; + } + + return idx; +} + + int -qemuAssignDeviceChrAlias(virDomainDefPtr def ATTRIBUTE_UNUSED, +qemuAssignDeviceChrAlias(virDomainDefPtr def, virDomainChrDefPtr chr, ssize_t idx) { @@ -891,6 +924,9 @@ qemuAssignDeviceChrAlias(virDomainDefPtr def ATTRIBUTE_UNUSED, return -1; } + if (idx == -1 && (idx = qemuGetNextChrDevIndex(def, chr, prefix)) < 0) + return -1; + return virAsprintf(&chr->info.alias, "%s%zd", prefix, idx); } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d858131..486de4e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6387,6 +6387,13 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, dev->data.redirdev = NULL; break; + case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainAttachChrDevice(driver, vm, + dev->data.chr); + if (!ret) + dev->data.chr = NULL; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("device type '%s' cannot be attached"), @@ -6474,6 +6481,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_HOSTDEV: ret = qemuDomainDetachHostDevice(driver, vm, dev); break; + case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr); + break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("This type of device cannot be hot unplugged")); @@ -6890,7 +6900,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; + unsigned int affect, parse_flags = 0; virQEMUCapsPtr qemuCaps = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; @@ -6938,9 +6948,13 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, goto endjob; } + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && + !(flags & VIR_DOMAIN_AFFECT_LIVE)) + parse_flags |= VIR_DOMAIN_XML_INACTIVE; + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + parse_flags); if (dev == NULL) goto endjob; @@ -7168,7 +7182,7 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; + unsigned int affect, parse_flags = 0; virQEMUCapsPtr qemuCaps = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; @@ -7216,9 +7230,13 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, goto endjob; } + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && + !(flags & VIR_DOMAIN_AFFECT_LIVE)) + parse_flags |= VIR_DOMAIN_XML_INACTIVE; + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + parse_flags); if (dev == NULL) goto endjob; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cc8779d..237afe9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1160,6 +1160,67 @@ error: } +int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + char *devstr = NULL; + char *charAlias = NULL; + bool remove = false; + + if (virDomainChrFind(vmdef, chr)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("chardev already exists")); + return ret; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return ret; + } + + if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) + return ret; + + if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) + return ret; + + if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virDomainChrInsert(vmdef, chr) < 0) + goto cleanup; + remove = true; + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + + if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) { + /* detach associated chardev on error */ + qemuMonitorDetachCharDev(priv->mon, charAlias); + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm); + + ret = 0; +cleanup: + if (ret < 0 && remove) + virDomainChrRemove(vmdef, chr); + VIR_FREE(charAlias); + VIR_FREE(devstr); + return ret; +} + int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -2992,3 +3053,50 @@ int qemuDomainDetachLease(virQEMUDriverPtr driver, virDomainLeaseDefFree(det_lease); return 0; } + +int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + virDomainChrDefPtr tmpChr; + char *charAlias = NULL; + char *devstr = NULL; + + if (!(tmpChr = virDomainChrFind(vmdef, chr))) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return ret; + } + + if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) + return ret; + + if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) { + virReportOOMError(); + return ret; + } + + qemuDomainObjEnterMonitor(driver, vm); + if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + + if (qemuMonitorDetachCharDev(priv->mon, charAlias) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm); + + virDomainChrRemove(vmdef, tmpChr); + virDomainChrDefFree(tmpChr); + ret = 0; + +cleanup: + VIR_FREE(devstr); + VIR_FREE(charAlias); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index da20eb1..46cd129 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -104,6 +104,12 @@ int qemuDomainAttachLease(virQEMUDriverPtr driver, int qemuDomainDetachLease(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainLeaseDefPtr lease); +int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr); +int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr); #endif /* __QEMU_HOTPLUG_H__ */ -- 1.8.1.5
On Wed, Jul 10, 2013 at 07:02:58PM +0200, Michal Privoznik wrote:
Since previous patches has prepared everything for us, we may now implement live hotplug of a character device. --- src/qemu/qemu_command.c | 38 ++++++++++++++++- src/qemu/qemu_driver.c | 26 ++++++++++-- src/qemu/qemu_hotplug.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 6 +++ 4 files changed, 173 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 063d76b..6842cab 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -863,8 +863,41 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx); }
+static ssize_t +qemuGetNextChrDevIndex(virDomainDefPtr def, + virDomainChrDefPtr chr, + const char *prefix) +{ + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + size_t i; + ssize_t idx = 0; + const char *prefix2 = NULL; + + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE) + prefix2 = "serial"; + + virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr); + + for (i = 0; i < *cntPtr; i++) { + ssize_t thisidx; + if (((thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix)) < 0) && + (prefix2 && + (thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix2)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to determine device index for character device")); + return -1; + } + if (thisidx >= idx) + idx = thisidx + 1; + } + + return idx; +} + + int -qemuAssignDeviceChrAlias(virDomainDefPtr def ATTRIBUTE_UNUSED, +qemuAssignDeviceChrAlias(virDomainDefPtr def, virDomainChrDefPtr chr, ssize_t idx) { @@ -891,6 +924,9 @@ qemuAssignDeviceChrAlias(virDomainDefPtr def ATTRIBUTE_UNUSED, return -1; }
+ if (idx == -1 && (idx = qemuGetNextChrDevIndex(def, chr, prefix)) < 0) + return -1; + return virAsprintf(&chr->info.alias, "%s%zd", prefix, idx); }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d858131..486de4e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6387,6 +6387,13 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, dev->data.redirdev = NULL; break;
+ case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainAttachChrDevice(driver, vm, + dev->data.chr); + if (!ret) + dev->data.chr = NULL; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("device type '%s' cannot be attached"), @@ -6474,6 +6481,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_HOSTDEV: ret = qemuDomainDetachHostDevice(driver, vm, dev); break; + case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr); + break;
As with the cold-plug code, I'm not sure this is handling of the case of adding/removing the first serial device, where you need to update the corresponding console device in virDomainDefPtr. Also we ought to probably forbid hotplug/unplug of a console device with type=serial, and require they change the actual serial device directly. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The test is currently testing just device update function. However, chardev hotplug is implemented just for device attach and detach. This fact means, the test needs to be rewritten (the majority of the code is still shared). Moreover, we are now able to pass VM among multiple test runs. So for instance, while we add a device in the first run, we can remove it in the second run. --- tests/qemuhotplugtest.c | 194 +++++++++++++++++---- .../qemuhotplug-console-virtio.xml | 5 + .../qemuxml2argv-console-compat-2.xml | 122 +++++++++++++ 3 files changed, 284 insertions(+), 37 deletions(-) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 15d70d2..d4971c2 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -33,11 +33,20 @@ static virQEMUDriver driver; +enum { + ATTACH, + DETACH, + UPDATE +}; + struct qemuHotplugTestData { const char *domain_filename; const char *device_filename; bool fail; const char *const *mon; + int action; + bool keep; + virDomainObjPtr vm; }; static int @@ -46,6 +55,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, const char *filename) { int ret = -1; + qemuDomainObjPrivatePtr priv = NULL; if (!(*vm = virDomainObjNew(xmlopt))) goto cleanup; @@ -57,12 +67,85 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, 0))) goto cleanup; + priv = (*vm)->privateData; + + if (!(priv->qemuCaps = virQEMUCapsNew())) + goto cleanup; + + /* for attach & detach qemu must support -device */ + virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE); + ret = 0; cleanup: return ret; } static int +testQemuHotplugAttach(virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainAttachChrDevice(&driver, vm, dev->data.chr); + break; + default: + if (virTestGetVerbose()) + fprintf(stderr, "device type '%s' cannot be attached", + virDomainDeviceTypeToString(dev->type)); + break; + } + + return ret; +} + +static int +testQemuHotplugDetach(virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainDetachChrDevice(&driver, vm, dev->data.chr); + break; + default: + if (virTestGetVerbose()) + fprintf(stderr, "device type '%s' cannot be attached", + virDomainDeviceTypeToString(dev->type)); + break; + } + + return ret; +} + +static int +testQemuHotplugUpdate(virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + + /* XXX Ideally, we would call qemuDomainUpdateDeviceLive here. But that + * would require us to provide virConnectPtr and virDomainPtr (they're used + * in case of updating a disk device. So for now, we will proceed with + * breaking the function into pieces. If we ever learn how to fake those + * required object, we can replace this code then. */ + switch (dev->type) { + case VIR_DOMAIN_DEVICE_GRAPHICS: + ret = qemuDomainChangeGraphics(&driver, vm, dev->data.graphics); + break; + default: + if (virTestGetVerbose()) + fprintf(stderr, "device type '%s' cannot be updated", + virDomainDeviceTypeToString(dev->type)); + break; + } + + return ret; +} + +static int testQemuHotplug(const void *data) { int ret = -1; @@ -72,6 +155,7 @@ testQemuHotplug(const void *data) char *device_xml = NULL; const char *const *tmp; bool fail = test->fail; + bool keep = test->keep; virDomainObjPtr vm = NULL; virDomainDeviceDefPtr dev = NULL; virCapsPtr caps = NULL; @@ -87,17 +171,18 @@ testQemuHotplug(const void *data) if (!(caps = virQEMUDriverGetCapabilities(&driver, false))) goto cleanup; - if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_filename) < 0) - goto cleanup; - - priv = vm->privateData; + if (test->vm) { + vm = test->vm; + } else { + if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_filename) < 0) + goto cleanup; + } if (virtTestLoadFile(device_filename, &device_xml) < 0) goto cleanup; if (!(dev = virDomainDeviceDefParse(device_xml, vm->def, - caps, driver.xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + caps, driver.xmlopt, 0))) goto cleanup; /* Now is the best time to feed the spoofed monitor with predefined @@ -117,6 +202,7 @@ testQemuHotplug(const void *data) goto cleanup; } + priv = vm->privateData; priv->mon = qemuMonitorTestGetMonitor(test_mon); priv->monJSON = true; @@ -125,20 +211,22 @@ testQemuHotplug(const void *data) * tries to lock it again */ virObjectUnlock(priv->mon); - /* XXX Ideally, we would call qemuDomainUpdateDeviceLive here. But that - * would require us to provide virConnectPtr and virDomainPtr (they're used - * in case of updating a disk device. So for now, we will proceed with - * breaking the function into pieces. If we ever learn how to fake those - * required object, we can replace this code then. */ - switch (dev->type) { - case VIR_DOMAIN_DEVICE_GRAPHICS: - ret = qemuDomainChangeGraphics(&driver, vm, dev->data.graphics); + switch (test->action) { + case ATTACH: + ret = testQemuHotplugAttach(vm, dev); + if (!ret) { + /* avoid @dev double free on success, + * as @dev is part of vm->def now */ + dev = NULL; + } break; - default: - if (virTestGetVerbose()) - fprintf(stderr, "device type '%s' cannot be updated", - virDomainDeviceTypeToString(dev->type)); + + case DETACH: + ret = testQemuHotplugDetach(vm, dev); break; + + case UPDATE: + ret = testQemuHotplugUpdate(vm, dev); } cleanup: @@ -148,7 +236,12 @@ cleanup: /* don't dispose test monitor with VM */ if (priv) priv->mon = NULL; - virObjectUnref(vm); + if (keep) { + test->vm = vm; + } else { + virObjectUnref(vm); + test->vm = NULL; + } virDomainDeviceDefFree(dev); virObjectUnref(caps); qemuMonitorTestFree(test_mon); @@ -159,6 +252,7 @@ static int mymain(void) { int ret = 0; + struct qemuHotplugTestData data = {0}; #if !WITH_YAJL fputs("libvirt not compiled with yajl, skipping this test\n", stderr); @@ -180,26 +274,52 @@ mymain(void) if (VIR_STRDUP_QUIET(driver.config->spicePassword, "123456") < 0) return EXIT_FAILURE; -#define DO_TEST(file, dev, fial, ...) \ - do { \ - const char *my_mon[] = { __VA_ARGS__, NULL}; \ - struct qemuHotplugTestData data = \ - {.domain_filename = file, .device_filename = dev, .fail = fial, \ - .mon = my_mon}; \ - if (virtTestRun(#file, 1, testQemuHotplug, &data) < 0) \ - ret = -1; \ +#define DO_TEST(file, dev, fial, kep, ...) \ + const char *my_mon[] = { __VA_ARGS__, NULL}; \ + data.domain_filename = file; \ + data.device_filename = dev; \ + data.fail = fial; \ + data.mon = my_mon; \ + data.keep = kep; \ + if (virtTestRun(#file, 1, testQemuHotplug, &data) < 0) \ + ret = -1; \ + +#define DO_TEST_ATTACH(file, dev, fial, kep, ...) \ + do { \ + data.action = ATTACH; \ + DO_TEST(file, dev, fial, kep, __VA_ARGS__) \ + } while (0) + +#define DO_TEST_DETACH(file, dev, fial, kep, ...) \ + do { \ + data.action = DETACH; \ + DO_TEST(file, dev, fial, kep, __VA_ARGS__) \ + } while (0) + +#define DO_TEST_UPDATE(file, dev, fial, kep, ...) \ + do { \ + data.action = UPDATE; \ + DO_TEST(file, dev, fial, kep, __VA_ARGS__) \ } while (0) - DO_TEST("graphics-spice", "graphics-spice-nochange", false, NULL); - DO_TEST("graphics-spice-timeout", "graphics-spice-timeout-nochange", false, - "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); - DO_TEST("graphics-spice-timeout", "graphics-spice-timeout-password", false, - "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); - DO_TEST("graphics-spice", "graphics-spice-listen", true, NULL); - DO_TEST("graphics-spice-listen-network", "graphics-spice-listen-network", false, - "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); - /* Strange huh? Currently, only graphics can be testet :-P */ - DO_TEST("disk-cdrom", "disk-cdrom-nochange", true, NULL); + DO_TEST_UPDATE("graphics-spice", "graphics-spice-nochange", false, false, NULL); + DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-nochange", false, false, + "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); + DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-password", false, false, + "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); + DO_TEST_UPDATE("graphics-spice", "graphics-spice-listen", true, false, NULL); + DO_TEST_UPDATE("graphics-spice-listen-network", "graphics-spice-listen-network", false, false, + "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); + /* Strange huh? Currently, only graphics can be updated :-P */ + DO_TEST_UPDATE("disk-cdrom", "disk-cdrom-nochange", true, false, NULL); + + DO_TEST_ATTACH("console-compat-2", "console-virtio", false, true, + "chardev-add", "{\"return\": {\"pty\": \"/dev/pts/26\"}}", + "device_add", "{\"return\": {}}"); + + DO_TEST_DETACH("console-compat-2", "console-virtio", false, false, + "device_del", "{\"return\": {}}", + "chardev-remove", "{\"return\": {}}"); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); diff --git a/tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml b/tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml new file mode 100644 index 0000000..bfab6ff --- /dev/null +++ b/tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml @@ -0,0 +1,5 @@ + <console type='pty'> + <source path='/dev/pts/26'/> + <target type='virtio' port='1'/> + <alias name='console1'/> + </console> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml b/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml new file mode 100644 index 0000000..3fc1153 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml @@ -0,0 +1,122 @@ +<domain type='kvm' id='2'> + <name>f17</name> + <uuid>a1cd52eb-d37f-4717-fc6e-972f0774f4c9</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <resource> + <partition>/machine</partition> + </resource> + <os> + <type arch='x86_64' machine='pc-i440fx-1.4'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='yes'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source file='/var/lib/libvirt/images/f17.qcow2'/> + <target dev='vda' bus='virtio'/> + <alias name='virtio-disk0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw' cache='none'/> + <source file='/home/user/tmp/Fedora-17-x86_64-Live-KDE.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <alias name='ide0-1-0'/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'> + <alias name='ide0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='usb' index='0'> + <alias name='usb0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <alias name='pci0'/> + </controller> + <controller type='virtio-serial' index='0'> + <alias name='virtio-serial0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </controller> + <interface type='network'> + <mac address='52:54:00:ea:35:6f'/> + <source network='default'/> + <target dev='vnet0'/> + <model type='virtio'/> + <bandwidth> + <inbound average='4000' peak='8000' floor='200' burst='1024'/> + <outbound average='4000' peak='8000' burst='1024'/> + </bandwidth> + <alias name='net0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + <serial type='pty'> + <source path='/dev/pts/22'/> + <target type='isa-serial' port='0'/> + <alias name='serial0'/> + </serial> + <serial type='pty'> + <source path='/dev/pts/25'/> + <target port='0'/> + <alias name='serial1'/> + </serial> + <serial type='tcp'> + <source mode='bind' host='0.0.0.0' service='2445'/> + <protocol type='raw'/> + <target port='1'/> + <alias name='serial2'/> + </serial> + <console type='pty' tty='/dev/pts/22'> + <source path='/dev/pts/22'/> + <target type='serial' port='0'/> + <alias name='serial0'/> + </console> + <channel type='unix'> + <source mode='bind' path='/var/lib/libvirt/qemu/f17x86_64.agent'/> + <target type='virtio' name='org.qemu.guest_agent.0'/> + <alias name='channel0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <input type='tablet' bus='usb'> + <alias name='input0'/> + </input> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='5900' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + <sound model='ich6'> + <alias name='sound0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </sound> + <video> + <model type='cirrus' vram='9216' heads='1'/> + <alias name='video0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'> + <alias name='balloon0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </memballoon> + </devices> + <seclabel type='static' model='dac' relabel='no'> + <label>root:root</label> + </seclabel> +</domain> -- 1.8.1.5
participants (4)
-
Chris Evich -
Daniel P. Berrange -
John Ferlan -
Michal Privoznik