[libvirt] [PATCH v3 0/9] Chardev hotplug
The third 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 | 154 +++++++++++- src/conf/domain_conf.h | 15 ++ src/libvirt_private.syms | 4 + src/qemu/qemu_command.c | 277 +++++++++++++++++---- src/qemu/qemu_command.h | 15 +- src/qemu/qemu_driver.c | 51 +++- 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 | 252 +++++++++++++++++-- src/qemu/qemu_monitor_json.h | 5 + tests/qemuhotplugtest.c | 194 ++++++++++++--- .../qemuhotplug-console-virtio.xml | 5 + .../qemuxml2argv-console-compat-2.xml | 122 +++++++++ 15 files changed, 1134 insertions(+), 120 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 | 135 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 15 ++++++ src/libvirt_private.syms | 4 ++ 3 files changed, 154 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 21ffc8f..f82ee62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10084,6 +10084,141 @@ 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); + + if (VIR_APPEND_ELEMENT(*arrPtr, *cntPtr, chr) < 0) { + virReportOOMError(); + return -1; + } + + return 0; +} + +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 2b55de5..b0d7a6a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2386,6 +2386,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 281478f..266329c 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 Tue, Jul 02, 2013 at 05:53:00PM +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 | 135 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 15 ++++++ src/libvirt_private.syms | 4 ++ 3 files changed, 154 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 :|
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 f82ee62..1d40c76 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1758,10 +1758,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: @@ -9461,6 +9463,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; @@ -18031,9 +18044,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 4856f37..6213ab4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6455,6 +6455,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, virDomainHostdevDefPtr hostdev; virDomainLeaseDefPtr lease; virDomainControllerDefPtr controller; + virDomainChrDefPtr chr; switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -6536,10 +6537,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; } @@ -6554,6 +6568,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]; @@ -6621,6 +6636,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 Tue, Jul 02, 2013 at 05:53:01PM +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. --- src/conf/domain_conf.c | 19 +++++++++++++++++-- src/qemu/qemu_driver.c | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 47 insertions(+), 5 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 :|
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 | 47 ++++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c0d7960..cd6e268 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4773,6 +4773,32 @@ no_memory: goto cleanup; } +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: + virReportOOMError(); + virJSONValueFree(data); + virJSONValueFree(addr); + return NULL; +} + int qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon, const char *host, @@ -4781,28 +4807,16 @@ 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)) { - virReportOOMError(); - 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) { + if (virAsprintf(&port_str, "%u", port) < 0) { virReportOOMError(); - goto cleanup; + 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, @@ -4825,7 +4839,6 @@ cleanup: virJSONValueFree(reply); virJSONValueFree(cmd); virJSONValueFree(addr); - virJSONValueFree(data); return ret; } -- 1.8.1.5
On Tue, Jul 02, 2013 at 05:53:02PM +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 | 47 ++++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 17 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 + 4 files changed, 209 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 091e239..3c45cf4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3626,3 +3626,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 86ef635..b382df3 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -697,6 +697,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 cd6e268..1d6f25c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4799,6 +4799,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, @@ -4978,3 +5001,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 */ -- 1.8.1.5
On Tue, Jul 02, 2013 at 05:53:03PM +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 + 4 files changed, 209 insertions(+)
Additions to the monitor APIs can be tested via the qemumonitorjsontest.c test case. It'd be desirable to have coverage of all the chardev source types involved here. ACK, in case you prefer to add the test cases as a separate patch. 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 ++ 4 files changed, 47 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 3c45cf4..db65c0b 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3647,3 +3647,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 b382df3..f87ca60 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -700,6 +700,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 1d6f25c..d9fdd95 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5160,3 +5160,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 */ -- 1.8.1.5
On Tue, Jul 02, 2013 at 05:53:04PM +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 ++ 4 files changed, 47 insertions(+)
ACK, but again this could use a simple test case. 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 | 75 +++++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_command.h | 3 ++ 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ba93233..903839f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -892,6 +892,65 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return 0; } +int +qemuAssignDeviceChrAlias(virDomainDefPtr def, + virDomainChrDefPtr chr, + ssize_t idx) +{ + const char *prefix = NULL; + const char *prefix2 = 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"; + prefix2 = "serial"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + prefix = "channel"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: + return -1; + } + + if (idx == -1) { + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + size_t i; + idx = 0; + + virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr); + + for (i = 0; i < *cntPtr; i++) { + int 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; + } + } + + if (virAsprintf(&chr->info.alias, "%s%zd", prefix, idx) < 0) { + virReportOOMError(); + return -1; + } + + return 0; +} int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) @@ -947,20 +1006,20 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) goto no_memory; } for (i = 0; i < def->nparallels; i++) { - if (virAsprintf(&def->parallels[i]->info.alias, "parallel%d", i) < 0) - goto no_memory; + 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) - goto no_memory; + 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) - goto no_memory; + 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) - goto no_memory; + if (qemuAssignDeviceChrAlias(def, def->consoles[i], i) < 0) + return -1; } for (i = 0; i < def->nhubs; i++) { if (virAsprintf(&def->hubs[i]->info.alias, "hub%d", i) < 0) 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 Tue, Jul 02, 2013 at 05:53:05PM +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 | 75 +++++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_command.h | 3 ++ 2 files changed, 70 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ba93233..903839f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -892,6 +892,65 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return 0; }
+int +qemuAssignDeviceChrAlias(virDomainDefPtr def, + virDomainChrDefPtr chr, + ssize_t idx) +{ + const char *prefix = NULL; + const char *prefix2 = 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"; + prefix2 = "serial"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + prefix = "channel"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: + return -1; + } + + if (idx == -1) { + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + size_t i; + idx = 0; + + virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr); + + for (i = 0; i < *cntPtr; i++) { + int 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; + } + }
The commit message describes this as a simple refactoring, but this if (idx== -1) {...} is all new functionality compared to what is being replaced. I'm not too sure that this logic is correct either when dealing with <console> with a 'serialXX' alias. It kind of feels like this if() block should be a separate function "qemuGetNextChrDevIndex" or something like that.
+ + if (virAsprintf(&chr->info.alias, "%s%zd", prefix, idx) < 0) { + virReportOOMError(); + return -1; + } + + return 0; +}
int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) @@ -947,20 +1006,20 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) goto no_memory; } for (i = 0; i < def->nparallels; i++) { - if (virAsprintf(&def->parallels[i]->info.alias, "parallel%d", i) < 0) - goto no_memory; + 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) - goto no_memory; + 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) - goto no_memory; + 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) - goto no_memory; + if (qemuAssignDeviceChrAlias(def, def->consoles[i], i) < 0) + return -1; } for (i = 0; i < def->nhubs; i++) { if (virAsprintf(&def->hubs[i]->info.alias, "hub%d", i) < 0) 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,
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 03.07.2013 17:29, Daniel P. Berrange wrote:
On Tue, Jul 02, 2013 at 05:53:05PM +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 | 75 +++++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_command.h | 3 ++ 2 files changed, 70 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ba93233..903839f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -892,6 +892,65 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return 0; }
+int +qemuAssignDeviceChrAlias(virDomainDefPtr def, + virDomainChrDefPtr chr, + ssize_t idx) +{ + const char *prefix = NULL; + const char *prefix2 = 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"; + prefix2 = "serial"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + prefix = "channel"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: + return -1; + } + + if (idx == -1) { + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + size_t i; + idx = 0; + + virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr); + + for (i = 0; i < *cntPtr; i++) { + int 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; + } + }
The commit message describes this as a simple refactoring, but this if (idx== -1) {...} is all new functionality compared to what is being replaced. I'm not too sure that this logic is correct either when dealing with <console> with a 'serialXX' alias.
This function just imitates other functions we've already: qemuAssignDeviceRedirdevAlias, qemuAssignDeviceHostdevAlias being examples. One can find even more. And regarding <console> I've sent 2 patches, none of them was accepted. So I had to workaround the problem in my patch. What's the solution you're suggesting? Just to refresh our memory: for <console> the alias can be either 'consoleX' or 'serialX' depending if the daemon was restarted or not. Michal
On Wed, Jul 10, 2013 at 02:57:48PM +0200, Michal Privoznik wrote:
On 03.07.2013 17:29, Daniel P. Berrange wrote:
On Tue, Jul 02, 2013 at 05:53:05PM +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 | 75 +++++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_command.h | 3 ++ 2 files changed, 70 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ba93233..903839f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -892,6 +892,65 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return 0; }
+int +qemuAssignDeviceChrAlias(virDomainDefPtr def, + virDomainChrDefPtr chr, + ssize_t idx) +{ + const char *prefix = NULL; + const char *prefix2 = 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"; + prefix2 = "serial"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + prefix = "channel"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: + return -1; + } + + if (idx == -1) { + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + size_t i; + idx = 0; + + virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr); + + for (i = 0; i < *cntPtr; i++) { + int 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; + } + }
The commit message describes this as a simple refactoring, but this if (idx== -1) {...} is all new functionality compared to what is being replaced. I'm not too sure that this logic is correct either when dealing with <console> with a 'serialXX' alias.
This function just imitates other functions we've already: qemuAssignDeviceRedirdevAlias, qemuAssignDeviceHostdevAlias being examples. One can find even more.
Sure, that's not what I was complaining about though. The issue is that you're mixing plain refactoring of code, with the inclusion of extra functionality. Nothing in this patch ever passes a value 'idx == -1' so code to handle that scenario is not related to refactoring. It can be introduced in whatever patch actually needs that code.
And regarding <console> I've sent 2 patches, none of them was accepted. So I had to workaround the problem in my patch. What's the solution you're suggesting? Just to refresh our memory: for <console> the alias can be either 'consoleX' or 'serialX' depending if the daemon was restarted or not.
I described what I think needs fixing here: https://www.redhat.com/archives/libvir-list/2013-July/msg00107.html
IIUC, this patch is intended to change things so that after libvirtd is restarted, we get:
def->seriales[0]->info == "serial0" def->consoles[0]->info == "console0"
but this is fixing the wrong thing. There is only one physical device emulated in the guest, which is a serial port with id==serial0, and this is reflected correctly in the XML we generate. Only the internal struct is different.
So what needs fixing is the code which populated def->consoles[0]->info with "console0" instead of the correct "serial0" string at VM startup.
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 10.07.2013 15:18, Daniel P. Berrange wrote:
On Wed, Jul 10, 2013 at 02:57:48PM +0200, Michal Privoznik wrote:
On 03.07.2013 17:29, Daniel P. Berrange wrote:
On Tue, Jul 02, 2013 at 05:53:05PM +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 | 75 +++++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_command.h | 3 ++ 2 files changed, 70 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ba93233..903839f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -892,6 +892,65 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return 0; }
+int +qemuAssignDeviceChrAlias(virDomainDefPtr def, + virDomainChrDefPtr chr, + ssize_t idx) +{ + const char *prefix = NULL; + const char *prefix2 = 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"; + prefix2 = "serial"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: + prefix = "channel"; + break; + + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: + return -1; + } + + if (idx == -1) { + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + size_t i; + idx = 0; + + virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr); + + for (i = 0; i < *cntPtr; i++) { + int 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; + } + }
The commit message describes this as a simple refactoring, but this if (idx== -1) {...} is all new functionality compared to what is being replaced. I'm not too sure that this logic is correct either when dealing with <console> with a 'serialXX' alias.
This function just imitates other functions we've already: qemuAssignDeviceRedirdevAlias, qemuAssignDeviceHostdevAlias being examples. One can find even more.
Sure, that's not what I was complaining about though. The issue is that you're mixing plain refactoring of code, with the inclusion of extra functionality. Nothing in this patch ever passes a value 'idx == -1' so code to handle that scenario is not related to refactoring. It can be introduced in whatever patch actually needs that code.
And regarding <console> I've sent 2 patches, none of them was accepted. So I had to workaround the problem in my patch. What's the solution you're suggesting? Just to refresh our memory: for <console> the alias can be either 'consoleX' or 'serialX' depending if the daemon was restarted or not.
I described what I think needs fixing here:
https://www.redhat.com/archives/libvir-list/2013-July/msg00107.html
IIUC, this patch is intended to change things so that after libvirtd is restarted, we get:
def->seriales[0]->info == "serial0" def->consoles[0]->info == "console0"
but this is fixing the wrong thing. There is only one physical device emulated in the guest, which is a serial port with id==serial0, and this is reflected correctly in the XML we generate. Only the internal struct is different.
So what needs fixing is the code which populated def->consoles[0]->info with "console0" instead of the correct "serial0" string at VM startup.
Daniel
Now that I am re-thinking this over, it's not a bug anymore. Libvirt already supports multiple consoles, and not all of them are just an alias to a serial console. That means, def->consoles[i]->info can be either "serialX" or "consoleX" regardless of my fix. This means, I have to deal with both aliases in this patch. But okay, I'll leave out the 'if (idx == -1) {}', and add it in later patch when needed. As a separate function. Michal
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 903839f..f1caffb 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6702,6 +6702,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, }; @@ -7779,13 +7794,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))) @@ -7816,10 +7826,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))) @@ -7833,8 +7841,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: @@ -7853,17 +7859,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: @@ -7889,12 +7888,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; } } @@ -7926,11 +7921,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: @@ -7948,12 +7940,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: @@ -8650,11 +8638,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; @@ -8686,7 +8675,7 @@ qemuBuildChrDeviceStr(virDomainChrDefPtr serial, } if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) - goto error; + goto error; } } @@ -8695,13 +8684,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
Since previous patches has prepared everything for us, we may now implement live hotplug of a character device. --- src/qemu/qemu_driver.c | 18 +++++++- src/qemu/qemu_hotplug.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 6 +++ 3 files changed, 130 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6213ab4..df54c64 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6235,6 +6235,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"), @@ -6322,6 +6329,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")); @@ -7022,7 +7032,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; @@ -7070,9 +7080,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 46875ad..af766d0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1192,6 +1192,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) @@ -3034,3 +3095,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
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 (2)
-
Daniel P. Berrange -
Michal Privoznik