[libvirt] [PATCH v2 0/8] Chardev hotplug
The second round which is just a rebase of the first round: https://www.redhat.com/archives/libvir-list/2013-May/msg00395.html Michal Privoznik (8): 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 src/conf/domain_conf.c | 182 +++++++++++++++++++++++++++++- src/conf/domain_conf.h | 11 ++ src/libvirt_private.syms | 4 + src/qemu/qemu_command.c | 262 +++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.h | 14 ++- src/qemu/qemu_driver.c | 43 ++++++- src/qemu/qemu_hotplug.c | 102 +++++++++++++++++ src/qemu/qemu_hotplug.h | 6 + src/qemu/qemu_monitor.c | 41 +++++++ src/qemu/qemu_monitor.h | 5 + src/qemu/qemu_monitor_json.c | 259 +++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_monitor_json.h | 5 + 12 files changed, 861 insertions(+), 73 deletions(-) -- 1.8.2.1
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 | 160 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 11 ++++ src/libvirt_private.syms | 4 ++ 3 files changed, 175 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a16ebd1..37f0ce9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10056,6 +10056,166 @@ virDomainLeaseRemove(virDomainDefPtr def, return virDomainLeaseRemoveAt(def, i); } +static bool +virDomainChrEquals(virDomainChrDefPtr src, + virDomainChrDefPtr tgt) +{ + if (!src || !tgt) + return src == tgt; + + if (!virDomainChrSourceDefIsEqual(&src->source, &tgt->source)) + return false; + + if (src->targetTypeAttr != tgt->targetTypeAttr) + return false; + + if (!src->targetTypeAttr) + return true; + + 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: + default: + return true; + } +} + +virDomainChrDefPtr +virDomainChrFind(virDomainDefPtr def, + virDomainChrDefPtr target) +{ + size_t i; + + for (i = 0; i < def->nparallels; i++) { + virDomainChrDefPtr chr = def->parallels[i]; + + if (virDomainChrEquals(chr, target)) + return chr; + } + + for (i = 0; i < def->nserials; i++) { + virDomainChrDefPtr chr = def->serials[i]; + + if (virDomainChrEquals(chr, target)) + return chr; + } + + + for (i = 0; i < def->nconsoles; i++) { + virDomainChrDefPtr chr = def->consoles[i]; + + if (virDomainChrEquals(chr, target)) + return chr; + } + + for (i = 0; i < def->nchannels; i++) { + virDomainChrDefPtr chr = def->channels[i]; + + if (virDomainChrEquals(chr, target)) + return chr; + } + + return NULL; +} + +int +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; + + default: + virReportError(VIR_ERR_OPERATION_INVALID, + _("Unsupported device type '%d'"), + chr->deviceType); + return -1; + } + return 0; +} + +int virDomainChrInsert(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + + if (virDomainChrGetDomainPtrs(vmdef, chr, &arrPtr, &cntPtr) < 0) + return -1; + + if (VIR_REALLOC_N(*arrPtr, *cntPtr + 1) < 0) { + virReportOOMError(); + return -1; + } + + (*arrPtr)[*cntPtr] = chr; + (*cntPtr)++; + + return 0; +} + +int virDomainChrRemove(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + virDomainChrDefPtr **arrPtr; + size_t i, *cntPtr; + + if (virDomainChrGetDomainPtrs(vmdef, chr, &arrPtr, &cntPtr) < 0) + return -1; + + for (i = 0; i < *cntPtr; i++) { + virDomainChrDefPtr tmp = (*arrPtr)[i]; + + if (virDomainChrEquals(tmp, chr)) + break; + } + + if (i == *cntPtr) + return -1; + + virDomainChrDefFree((*arrPtr)[i]); + if (*cntPtr > 1) { + memmove(*arrPtr + i, + *arrPtr + i + 1, + sizeof(**arrPtr) * (*cntPtr - (i + 1))); + ignore_value(VIR_REALLOC_N(*arrPtr, *cntPtr - 1)); + } else { + VIR_FREE(*arrPtr); + } + (*cntPtr)--; + + return 0; +} char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3a71d6c..6d650ff 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2355,6 +2355,17 @@ virDomainLeaseDefPtr virDomainLeaseRemove(virDomainDefPtr def, virDomainLeaseDefPtr lease); +int virDomainChrGetDomainPtrs(virDomainDefPtr vmdef, + virDomainChrDefPtr chr, + virDomainChrDefPtr ***arrPtr, + size_t **cntPtr); +virDomainChrDefPtr virDomainChrFind(virDomainDefPtr def, + virDomainChrDefPtr target); +int virDomainChrInsert(virDomainDefPtr vmdef, + virDomainChrDefPtr chr); +int 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 b93629f..b9ba731 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -77,6 +77,10 @@ virDomainChrConsoleTargetTypeToString; virDomainChrDefForeach; virDomainChrDefFree; virDomainChrDefNew; +virDomainChrFind; +virDomainChrGetDomainPtrs; +virDomainChrInsert; +virDomainChrRemove; virDomainChrSerialTargetTypeFromString; virDomainChrSerialTargetTypeToString; virDomainChrSourceDefCopy; -- 1.8.2.1
On 06/06/2013 02:29 PM, 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 | 160 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 11 ++++ src/libvirt_private.syms | 4 ++ 3 files changed, 175 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a16ebd1..37f0ce9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10056,6 +10056,166 @@ virDomainLeaseRemove(virDomainDefPtr def, return virDomainLeaseRemoveAt(def, i); }
+static bool +virDomainChrEquals(virDomainChrDefPtr src, + virDomainChrDefPtr tgt) +{ + if (!src || !tgt) + return src == tgt; + + if (!virDomainChrSourceDefIsEqual(&src->source, &tgt->source)) + return false; + + if (src->targetTypeAttr != tgt->targetTypeAttr) + return false; + + if (!src->targetTypeAttr) + return true; + + switch ((enum virDomainChrChannelTargetType) src->targetType) {
There should be one more check and switch above this one to differentiate according to src->deviceType and tgt->deviceType.
+ 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: + default:
No need for default case in here.
+ return true; + } +} + +virDomainChrDefPtr +virDomainChrFind(virDomainDefPtr def, + virDomainChrDefPtr target) +{ + size_t i; + + for (i = 0; i < def->nparallels; i++) { + virDomainChrDefPtr chr = def->parallels[i]; + + if (virDomainChrEquals(chr, target)) + return chr; + } + + for (i = 0; i < def->nserials; i++) { + virDomainChrDefPtr chr = def->serials[i]; + + if (virDomainChrEquals(chr, target)) + return chr; + } + + + for (i = 0; i < def->nconsoles; i++) { + virDomainChrDefPtr chr = def->consoles[i]; + + if (virDomainChrEquals(chr, target)) + return chr; + } + + for (i = 0; i < def->nchannels; i++) { + virDomainChrDefPtr chr = def->channels[i]; + + if (virDomainChrEquals(chr, target)) + return chr; + } +
You're iterating over all devices in the domain, but you can check which ones to check based on target->deviceType.
+ return NULL; +} + +int +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; + + default:
I'd rather cast the switch argument, put a TYPE_LAST here and you don't have to report the error then.
+ virReportError(VIR_ERR_OPERATION_INVALID, + _("Unsupported device type '%d'"), + chr->deviceType); + return -1; + } + return 0; +} +
And with this function, you can iterate over those devices in virDomainChrFind even more clearly.
+int virDomainChrInsert(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + + if (virDomainChrGetDomainPtrs(vmdef, chr, &arrPtr, &cntPtr) < 0) + return -1; + + if (VIR_REALLOC_N(*arrPtr, *cntPtr + 1) < 0) { + virReportOOMError(); + return -1; + } + + (*arrPtr)[*cntPtr] = chr; + (*cntPtr)++; +
VIR_APPEND_ELEMENT could make this a bit shorter and readable.
+ return 0; +} + +int virDomainChrRemove(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + virDomainChrDefPtr **arrPtr; + size_t i, *cntPtr; + + if (virDomainChrGetDomainPtrs(vmdef, chr, &arrPtr, &cntPtr) < 0) + return -1; + + for (i = 0; i < *cntPtr; i++) { + virDomainChrDefPtr tmp = (*arrPtr)[i]; + + if (virDomainChrEquals(tmp, chr)) + break; + } + + if (i == *cntPtr) + return -1; +
This is another part of code you can de-duplicate since it is similar to the virDomainChrFind (and deals with the problem mentioned there).
+ virDomainChrDefFree((*arrPtr)[i]); + if (*cntPtr > 1) { + memmove(*arrPtr + i, + *arrPtr + i + 1, + sizeof(**arrPtr) * (*cntPtr - (i + 1))); + ignore_value(VIR_REALLOC_N(*arrPtr, *cntPtr - 1)); + } else { + VIR_FREE(*arrPtr); + } + (*cntPtr)--; +
And for this you should be able to use the VIR_DELETE_ELEMENT macro.
+ return 0; +}
char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3a71d6c..6d650ff 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2355,6 +2355,17 @@ virDomainLeaseDefPtr virDomainLeaseRemove(virDomainDefPtr def, virDomainLeaseDefPtr lease);
+int virDomainChrGetDomainPtrs(virDomainDefPtr vmdef,
s/ /\n/
+ virDomainChrDefPtr chr, + virDomainChrDefPtr ***arrPtr, + size_t **cntPtr); +virDomainChrDefPtr virDomainChrFind(virDomainDefPtr def,
dtto
+ virDomainChrDefPtr target); +int virDomainChrInsert(virDomainDefPtr vmdef,
dtto
+ virDomainChrDefPtr chr); +int virDomainChrRemove(virDomainDefPtr vmdef,
dtto
+ virDomainChrDefPtr chr); + int virDomainSaveXML(const char *configDir,
pre-existing, but could be fixed.
virDomainDefPtr def, const char *xml); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b93629f..b9ba731 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -77,6 +77,10 @@ virDomainChrConsoleTargetTypeToString; virDomainChrDefForeach; virDomainChrDefFree; virDomainChrDefNew; +virDomainChrFind; +virDomainChrGetDomainPtrs; +virDomainChrInsert; +virDomainChrRemove; virDomainChrSerialTargetTypeFromString; virDomainChrSerialTargetTypeToString; virDomainChrSourceDefCopy;
Martin
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 what this patch exactly 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 | 22 ++++++++++++++++++++-- src/qemu/qemu_driver.c | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 37f0ce9..582eade 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1755,10 +1755,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: @@ -9433,6 +9435,20 @@ 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; + /* In case of serial chardev we want to parse <source> as it is the only + * thing distinguishing two serial chardevs */ + flags &= ~VIR_DOMAIN_XML_INACTIVE; + 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; @@ -17823,9 +17839,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 db56823..9ce186e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6110,6 +6110,7 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, virDomainHostdevDefPtr hostdev; virDomainLeaseDefPtr lease; virDomainControllerDefPtr controller; + virDomainChrDefPtr chr; switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -6191,10 +6192,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; } @@ -6209,6 +6223,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]; @@ -6276,6 +6291,18 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, break; + case VIR_DOMAIN_DEVICE_CHR: + chr = dev->data.chr; + if (virDomainChrRemove(vmdef, chr) < 0) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("device not present in domain configuration")); + return -1; + } + + virDomainChrDefFree(chr); + dev->data.chr = NULL; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("persistent detach of device is not supported")); -- 1.8.2.1
On 06/06/2013 02:29 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 what this patch exactly does. There is currently no implementation for a chardev
s/what this patch exactly does/exactly what this patch does/
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 | 22 ++++++++++++++++++++-- src/qemu/qemu_driver.c | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 37f0ce9..582eade 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1755,10 +1755,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: @@ -9433,6 +9435,20 @@ 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; + /* In case of serial chardev we want to parse <source> as it is the only + * thing distinguishing two serial chardevs */
The comment has no information value, we have to parse the XML anyway.
+ flags &= ~VIR_DOMAIN_XML_INACTIVE;
You're silently removing the flag without telling the user that. It's ok if you remove that in the last patch (which I haven't seen yet), but I guess error-ing out would be more appropriate. ACK, Martin
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 | 53 +++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 2b73884..b87b8b5 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4734,36 +4734,50 @@ no_memory: goto cleanup; } -int -qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon, - const char *host, - unsigned int port) +static virJSONValuePtr +qemuMonitorJSONBuildInetSocketAddress(const char *host, + const char *port) { - int ret = -1; - virJSONValuePtr cmd = NULL; - virJSONValuePtr reply = NULL; - virJSONValuePtr data = NULL; virJSONValuePtr addr = NULL; - char *port_str = NULL; + virJSONValuePtr data = NULL; if (!(data = virJSONValueNewObject()) || - !(addr = virJSONValueNewObject()) || - (virAsprintf(&port_str, "%u", port) < 0)) { - virReportOOMError(); - goto cleanup; - } + !(addr = virJSONValueNewObject())) + goto error; /* port is really expected as a string here by qemu */ if (virJSONValueObjectAppendString(data, "host", host) < 0 || - virJSONValueObjectAppendString(data, "port", port_str) < 0 || + virJSONValueObjectAppendString(data, "port", port) < 0 || virJSONValueObjectAppendString(addr, "type", "inet") < 0 || - virJSONValueObjectAppend(addr, "data", data) < 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, + unsigned int port) +{ + int ret = -1; + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + virJSONValuePtr addr = NULL; + char *port_str = NULL; + + 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, @@ -4786,7 +4800,6 @@ cleanup: virJSONValueFree(reply); virJSONValueFree(cmd); virJSONValueFree(addr); - virJSONValueFree(data); return ret; } -- 1.8.2.1
On Thu, Jun 06, 2013 at 14:30:00 +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 | 53 +++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 20 deletions(-)
ACK Jirka
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 | 189 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + 4 files changed, 216 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ad326c5..d11b97e 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 a607712..3e7b4cb 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -696,6 +696,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 b87b8b5..0399963 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4760,6 +4760,30 @@ error: return NULL; } +static virJSONValuePtr +qemuMonitorJSONBuildUnixSocketAddress(const char *path) +{ + 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, "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, @@ -4939,3 +4963,168 @@ 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; + 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: + default: + 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; +} + +static int +qemuMonitorJSONAttachCharDevInfo(virJSONValuePtr reply, + virDomainChrSourceDefPtr chr) +{ + virJSONValuePtr data; + const char *path; + + if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY) + return 0; + + if (!(data = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("chardev-add reply was missing return data")); + return -1; + } + + if (!(path = virJSONValueObjectGetString(data, "pty"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("chardev-add reply was missing pty path")); + return -1; + } + + if (VIR_STRDUP(chr->data.file.path, path) < 0) + return -1; + return 0; +} + +int +qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon, + const char *chrID, + virDomainChrSourceDefPtr chr) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + if (!(cmd = qemuMonitorJSONAttachCharDevCommand(chrID, chr))) + return ret; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + if (ret == 0) + ret = qemuMonitorJSONAttachCharDevInfo(reply, chr); + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 74e2476..b66fd3d 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.2.1
On Thu, Jun 06, 2013 at 14:30:01 +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 | 189 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + 4 files changed, 216 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ad326c5..d11b97e 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 a607712..3e7b4cb 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -696,6 +696,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 b87b8b5..0399963 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4760,6 +4760,30 @@ error: return NULL; }
+static virJSONValuePtr +qemuMonitorJSONBuildUnixSocketAddress(const char *path) +{ + virJSONValuePtr addr = NULL; + virJSONValuePtr data = NULL; + + if (!(data = virJSONValueNewObject()) || + !(addr = virJSONValueNewObject())) + goto error; + + /* port is really expected as a string here by qemu */
That's a bit surprising when unix sockets have no ports :-)
+ 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, @@ -4939,3 +4963,168 @@ 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; + 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";
To avoid adding an empty data object, I'd add: virJSONValueFree(data); data = NULL;
+ break; + + case VIR_DOMAIN_CHR_TYPE_PTY: + backend_type = "pty";
and here as well: virJSONValueFree(data); data = NULL;
+ 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: + default:
Remove the "default:" line.
+ 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)
Only append the data object if it's non-NULL (after freeing it when it's not required).
+ 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; +} + +static int +qemuMonitorJSONAttachCharDevInfo(virJSONValuePtr reply, + virDomainChrSourceDefPtr chr)
The name of this function is quite confusing since it doesn't really attach anything, it just parses the result of previous chardev-add command. I think it could actually be inlined in qemuMonitorJSONAttachCharDev to avoid the need for coming up with a better name :-)
+{ + virJSONValuePtr data; + const char *path; + + if (chr->type != VIR_DOMAIN_CHR_TYPE_PTY) + return 0; + + if (!(data = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("chardev-add reply was missing return data")); + return -1; + } + + if (!(path = virJSONValueObjectGetString(data, "pty"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("chardev-add reply was missing pty path")); + return -1; + } + + if (VIR_STRDUP(chr->data.file.path, path) < 0) + return -1; + return 0; +} + +int +qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon, + const char *chrID, + virDomainChrSourceDefPtr chr) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + if (!(cmd = qemuMonitorJSONAttachCharDevCommand(chrID, chr))) + return ret; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + if (ret == 0) + ret = qemuMonitorJSONAttachCharDevInfo(reply, chr); + + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +}
ACK with the small issues addressed. Jirka
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 d11b97e..6818256 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 3e7b4cb..373c7d9 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -699,6 +699,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 0399963..4b7e070 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5128,3 +5128,26 @@ qemuMonitorJSONAttachCharDev(qemuMonitorPtr mon, 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 b66fd3d..108c4a2 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.2.1
On Thu, Jun 06, 2013 at 14:30:02 +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 Jirka
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 | 66 ++++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_command.h | 3 +++ 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c4a162a..39b9d24 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -891,6 +891,64 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return 0; } +int +qemuAssignDeviceChrAlias(virDomainDefPtr def, + virDomainChrDefPtr chr, + size_t idx) +{ + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + const char *prefix; + + if (virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr) < 0) + return -1; + + 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; + + default: + virReportError(VIR_ERR_OPERATION_INVALID, + _("Unsupported device type '%d'"), + chr->deviceType); + return -1; + } + + if (idx == -1) { + size_t i; + idx = 0; + for (i = 0; i < *cntPtr; i++) { + int thisidx; + if ((thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix)) < 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%zu", prefix, idx) < 0) { + virReportOOMError(); + return -1; + } + + return 0; +} int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) @@ -946,19 +1004,19 @@ 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) + if (qemuAssignDeviceChrAlias(def, def->parallels[i], i) < 0) goto no_memory; } 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) goto no_memory; } 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) goto no_memory; } 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) goto no_memory; } for (i = 0; i < def->nhubs; i++) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2993448..900efd7 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, + size_t idx); int qemuParseKeywords(const char *str, -- 1.8.2.1
On Thu, Jun 06, 2013 at 14:30:03 +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 | 66 ++++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_command.h | 3 +++ 2 files changed, 65 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c4a162a..39b9d24 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -891,6 +891,64 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return 0; }
+int +qemuAssignDeviceChrAlias(virDomainDefPtr def, + virDomainChrDefPtr chr, + size_t idx)
Since you want to pass -1 as idx, you should declare it as signed
+{ + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + const char *prefix; + + if (virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr) < 0) + return -1; + + 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; + + default: + virReportError(VIR_ERR_OPERATION_INVALID, + _("Unsupported device type '%d'"), + chr->deviceType); + return -1; + } + + if (idx == -1) { + size_t i; + idx = 0; + for (i = 0; i < *cntPtr; i++) { + int thisidx; + if ((thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix)) < 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%zu", prefix, idx) < 0) {
and update %zu to be signed, too.
+ virReportOOMError(); + return -1; + } + + return 0; +}
ACK with the change. Jirka
On 06/06/2013 02:30 PM, 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_command.h | 3 +++ 2 files changed, 65 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c4a162a..39b9d24 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -891,6 +891,64 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return 0; }
+int +qemuAssignDeviceChrAlias(virDomainDefPtr def, + virDomainChrDefPtr chr, + size_t idx) +{ + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + const char *prefix; + + if (virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr) < 0) + return -1; + + 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; + + default: + virReportError(VIR_ERR_OPERATION_INVALID, + _("Unsupported device type '%d'"), + chr->deviceType); + return -1; + }
If you put _TYPE_LAST here you can drop the error message.
+ + if (idx == -1) { + size_t i; + idx = 0; + for (i = 0; i < *cntPtr; i++) { + int thisidx; + if ((thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix)) < 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%zu", prefix, idx) < 0) { + virReportOOMError(); + return -1; + } + + return 0; +}
int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) @@ -946,19 +1004,19 @@ 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) + if (qemuAssignDeviceChrAlias(def, def->parallels[i], i) < 0) goto no_memory; } 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) goto no_memory; } 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) goto no_memory; } 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) goto no_memory;
Double OOM reporting in all four cases.
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 | 196 ++++++++++++++++++++++++++++++++++++++---------- src/qemu/qemu_command.h | 11 ++- 2 files changed, 160 insertions(+), 47 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 39b9d24..ec44b4f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7767,12 +7767,9 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr); - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildChrDeviceStr(serial, qemuCaps, - def->os.arch, - def->os.machine))) + if (qemuBuildChrDeviceStr(&devstr, def, serial, qemuCaps) < 0) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-device", devstr, NULL); VIR_FREE(devstr); } else { virCommandAddArg(cmd, "-serial"); @@ -7804,10 +7801,10 @@ 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 (qemuBuildChrDeviceStr(&devstr, def, parallel, qemuCaps) < 0) + goto error; + virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); } else { virCommandAddArg(cmd, "-parallel"); if (!(devstr = qemuBuildChrArgStr(¶llel->source, NULL))) @@ -7821,8 +7818,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: @@ -7841,17 +7836,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: @@ -7877,11 +7865,9 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(devstr); } - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildVirtioSerialPortDevStr(channel, - qemuCaps))) + if (qemuBuildChrDeviceStr(&devstr, def, channel, qemuCaps) < 0) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-device", devstr, NULL); VIR_FREE(devstr); break; } @@ -7914,10 +7900,9 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr); - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildSclpDevStr(console))) + if (qemuBuildChrDeviceStr(&devstr, def, console, qemuCaps) < 0) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-device", devstr, NULL); VIR_FREE(devstr); break; @@ -7936,11 +7921,9 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr); - virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildVirtioSerialPortDevStr(console, - qemuCaps))) + if (qemuBuildChrDeviceStr(&devstr, def, console, qemuCaps) < 0) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-device", devstr, NULL); VIR_FREE(devstr); break; @@ -8638,11 +8621,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; @@ -8674,7 +8658,7 @@ qemuBuildChrDeviceStr(virDomainChrDefPtr serial, } if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) - goto error; + goto error; } } @@ -8683,13 +8667,143 @@ 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: + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unsupported channel target type %d"), + chr->targetType); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(addr); + return ret; +} + +static int +qemuBuildConsoleChrDeviceStr(char **deviceStr, + virDomainChrDefPtr chr, + virQEMUCapsPtr qemuCaps) +{ + int ret = -1; + + switch (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: + break; + + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported console target type %d"), + chr->targetType); + goto cleanup; + } + + 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; + + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Chardev deviceType %d is not handled"), + chr->deviceType); + break; + } + + 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 900efd7..c925ebf 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -75,12 +75,11 @@ 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.2.1
On 06/06/2013 02:30 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 | 196 ++++++++++++++++++++++++++++++++++++++---------- src/qemu/qemu_command.h | 11 ++- 2 files changed, 160 insertions(+), 47 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 39b9d24..ec44b4f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c [...] @@ -7936,11 +7921,9 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr);
- virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildVirtioSerialPortDevStr(console, - qemuCaps))) + if (qemuBuildChrDeviceStr(&devstr, def, console, qemuCaps) < 0) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-device", devstr, NULL); VIR_FREE(devstr); break;
There seems to be still a lot of code duplication. Can you move the '-device' creation out of the switch since you created such universal function?
@@ -8638,11 +8621,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;
@@ -8674,7 +8658,7 @@ qemuBuildChrDeviceStr(virDomainChrDefPtr serial, }
if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) - goto error; + goto error; } }
@@ -8683,13 +8667,143 @@ qemuBuildChrDeviceStr(virDomainChrDefPtr serial, goto error; }
- return virBufferContentAndReset(&cmd); + *deviceStr = virBufferContentAndReset(&cmd); + return 0;
- error: +error:
We should unify this and write it in the HACKING file, but something tells me we won't reach easy agreement. Would anyone be against a rule for labels having one space in front of them? git doesn't use it after function name when there's a space (plus it is the default indentation in my editor for labels).
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: + default:
Delete the default case and let the compiler do the check for missing target types. It's pre-existing, but I guess we could get rid of those _NONE types since those are not used anywhere, couldn't we?
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unsupported channel target type %d"), + chr->targetType); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(addr); + return ret; +} + +static int +qemuBuildConsoleChrDeviceStr(char **deviceStr, + virDomainChrDefPtr chr, + virQEMUCapsPtr qemuCaps) +{ + int ret = -1; + + switch (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: + break; + + default:
Cast the targetType to the appropriate enum and change this to _LAST.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported console target type %d"), + chr->targetType); + goto cleanup; + } + + 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; + + default:
s/default/VIR_DOMAIN_CHR_DEVICE_TYPE_LAST/
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("Chardev deviceType %d is not handled"),
I'd reword this to the usual 'unsupported device type %d'.
+ chr->deviceType); + break; + } + + return ret; +} + +
Unnecessary newline.
/* * 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 900efd7..c925ebf 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -75,12 +75,11 @@ 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,
s/ /\n/
+ virDomainDefPtr vmdef, + virDomainChrDefPtr chr, + virQEMUCapsPtr qemuCaps);
/* With vlan == -1, use netdev syntax, else old hostnet */ char * qemuBuildHostNetStr(virDomainNetDefPtr net,
ACK with mentioned changes fixed (I don't care about the label of course). Martin
Since previous patches has prepared everything for us, we may now implement live hotplug of a character device. --- src/qemu/qemu_driver.c | 10 +++++ src/qemu/qemu_hotplug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 6 +++ 3 files changed, 118 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9ce186e..745393a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5890,6 +5890,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"), @@ -5977,6 +5984,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")); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6c07af5..3e3bf2f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1192,6 +1192,63 @@ 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; + + 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; + } + + 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); + + if (virDomainChrInsert(vmdef, chr) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(charAlias); + VIR_FREE(devstr); + return ret; +} + int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -3015,3 +3072,48 @@ 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); + + ret = virDomainChrRemove(vmdef, tmpChr); + +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.2.1
On 06/06/2013 02:30 PM, Michal Privoznik wrote:
Since previous patches has prepared everything for us, we may now
s/has/have/
implement live hotplug of a character device. --- src/qemu/qemu_driver.c | 10 +++++ src/qemu/qemu_hotplug.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 6 +++ 3 files changed, 118 insertions(+)
[...]
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6c07af5..3e3bf2f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1192,6 +1192,63 @@ 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; + + 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; + } + + 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); + + if (virDomainChrInsert(vmdef, chr) < 0) + goto cleanup;
This shouldn't fail, but (and similarly to 'Insert'), do we want to keep the device to be kept there? To avoid those problems, you could move this code before the attaching the device in and just remove it from the DefPtr in case the attach isn't successful. You should do it similarly for 'Remove', but there's no place to fail, or is there? Also, you should use virDomainAuditChardevPath() when adding/removing the backend. ACK with those points addressed. Could you add a test for the hotplug as well? Martin
On 06.06.2013 14:29, Michal Privoznik wrote:
The second round which is just a rebase of the first round:
https://www.redhat.com/archives/libvir-list/2013-May/msg00395.html
Michal Privoznik (8): 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
src/conf/domain_conf.c | 182 +++++++++++++++++++++++++++++- src/conf/domain_conf.h | 11 ++ src/libvirt_private.syms | 4 + src/qemu/qemu_command.c | 262 +++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.h | 14 ++- src/qemu/qemu_driver.c | 43 ++++++- src/qemu/qemu_hotplug.c | 102 +++++++++++++++++ src/qemu/qemu_hotplug.h | 6 + src/qemu/qemu_monitor.c | 41 +++++++ src/qemu/qemu_monitor.h | 5 + src/qemu/qemu_monitor_json.c | 259 +++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_monitor_json.h | 5 + 12 files changed, 861 insertions(+), 73 deletions(-)
Ping?
participants (4)
-
Jiri Denemark -
Ján Tomko -
Martin Kletzander -
Michal Privoznik