Create a sorted array of virtio-serial controllers. Each of the elements contains the controller index and a bitmap of available ports. Buses are not tracked, because they aren't supported by QEMU. --- src/conf/domain_addr.c | 348 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_addr.h | 56 ++++++++ src/libvirt_private.syms | 9 ++ 3 files changed, 413 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index fb4a76f..d9d01fc 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -718,3 +718,351 @@ virDomainCCWAddressSetCreate(void) virDomainCCWAddressSetFree(addrs); return NULL; } + + +#define VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS 31 + + +/* virDomainVirtioSerialAddrSetCreate + * + * Allocates an address set for virtio serial addresses + */ +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreate(void) +{ + virDomainVirtioSerialAddrSetPtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + return ret; +} + +static void +virDomainVirtioSerialControllerFree(virDomainVirtioSerialControllerPtr cont) +{ + if (cont) { + virBitmapFree(cont->ports); + VIR_FREE(cont); + } +} + +static ssize_t +virDomainVirtioSerialAddrPlaceController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainVirtioSerialControllerPtr cont) +{ + size_t i; + + for (i = 0; i < addrs->ncontrollers; i++) { + if (addrs->controllers[i]->idx >= cont->idx) + return i; + } + return -1; +} + +static ssize_t +virDomainVirtioSerialAddrFindController(virDomainVirtioSerialAddrSetPtr addrs, + unsigned int idx) +{ + size_t i; + + for (i = 0; i < addrs->ncontrollers; i++) { + if (addrs->controllers[i]->idx == idx) + return i; + } + return -1; +} + +/* virDomainVirtioSerialAddrSetAddController + * + * Adds virtio serial ports of the existing controller + * to the address set. + */ +int +virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) +{ + int ret = -1; + int ports; + virDomainVirtioSerialControllerPtr cnt = NULL; + ssize_t insertAt; + + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + return 0; + + ports = cont->opts.vioserial.ports; + if (ports == -1) + ports = VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS; + + VIR_DEBUG("Adding virtio serial controller index %u with %d" + " ports to the address set", cont->idx, ports); + + if (VIR_ALLOC(cnt) < 0) + goto cleanup; + + if (!(cnt->ports = virBitmapNew(ports))) + goto cleanup; + cnt->idx = cont->idx; + + insertAt = virDomainVirtioSerialAddrPlaceController(addrs, cnt); + if (VIR_INSERT_ELEMENT(addrs->controllers, insertAt, + addrs->ncontrollers, cnt) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virDomainVirtioSerialControllerFree(cnt); + return ret; +} + +/* virDomainVirtioSerialAddrSetAddControllers + * + * Adds virtio serial ports of controllers present in the domain definition + * to the address set. + */ +int +virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + if (virDomainVirtioSerialAddrSetAddController(addrs, + def->controllers[i]) < 0) + return -1; + } + + return 0; +} + +/* virDomainVirtioSerialAddrSetRemoveController + * + * Removes a virtio serial controller from the address set. + */ +int +virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) +{ + int ret = -1; + ssize_t pos; + + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + return 0; + + VIR_DEBUG("Removing virtio serial controller index %u " + "from the address set", cont->idx); + + pos = virDomainVirtioSerialAddrFindController(addrs, cont->idx); + + if (pos >= 0 && + VIR_DELETE_ELEMENT(addrs->controllers, pos, addrs->ncontrollers) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} + +void +virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs) +{ + size_t i; + if (addrs) { + for (i = 0; i < addrs->ncontrollers; i++) + virDomainVirtioSerialControllerFree(addrs->controllers[i]); + VIR_FREE(addrs); + } +} + +static int +virDomainVirtioSerialAddrNext(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceVirtioSerialAddress *addr, + bool allowZero) +{ + int ret = -1; + ssize_t port, start = 0; + ssize_t i; + unsigned int controller; + + /* port number 0 is reserved for virtconsoles */ + if (allowZero) + start = -1; + + if (addrs->ncontrollers == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("no virtio-serial controllers are available")); + goto cleanup; + } + + for (i = 0; i < addrs->ncontrollers; i++) { + virBitmapPtr map = addrs->controllers[i]->ports; + if ((port = virBitmapNextClearBit(map, start)) >= 0) { + controller = addrs->controllers[i]->idx; + goto success; + } + } + + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Unable to find a free virtio-serial port")); + + cleanup: + return ret; + + success: + addr->bus = 0; + addr->port = port; + addr->controller = controller; + VIR_DEBUG("Found free virtio serial controller %u port %u", addr->controller, + addr->port); + ret = 0; + goto cleanup; +} + +/* virDomainVirtioSerialAddrAutoAssign + * + * reserve a virtio serial address of the device (if it has one) + * or assign a virtio serial address to the device + */ +int +virDomainVirtioSerialAddrAutoAssign(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info, + bool allowZero) +{ + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && + info->addr.vioserial.port) + return virDomainVirtioSerialAddrReserve(NULL, NULL, info, addrs); + else + return virDomainVirtioSerialAddrAssign(addrs, info, allowZero); +} + + +int +virDomainVirtioSerialAddrAssign(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info, + bool allowZero) +{ + int ret = -1; + virDomainDeviceInfo nfo = { NULL }; + virDomainDeviceInfoPtr ptr = allowZero ? &nfo : info; + + ptr->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL; + + if (virDomainVirtioSerialAddrNext(addrs, &ptr->addr.vioserial, + allowZero) < 0) + goto cleanup; + + if (virDomainVirtioSerialAddrReserve(NULL, NULL, ptr, addrs) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} + +/* virDomainVirtioSerialAddrReserve + * + * Reserve the virtio serial address of the device + * + * For use with virDomainDeviceInfoIterate, + * opaque should be the address set + */ +int +virDomainVirtioSerialAddrReserve(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *data) +{ + virDomainVirtioSerialAddrSetPtr addrs = data; + char *str = NULL; + int ret = -1; + virBitmapPtr map = NULL; + bool b; + ssize_t i; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL || + info->addr.vioserial.port == 0) + return 0; + + VIR_DEBUG("Reserving virtio serial %u %u", info->addr.vioserial.controller, + info->addr.vioserial.port); + + i = virDomainVirtioSerialAddrFindController(addrs, info->addr.vioserial.controller); + if (i < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u is missing"), + info->addr.vioserial.controller); + goto cleanup; + } + + map = addrs->controllers[i]->ports; + if (virBitmapGetBit(map, info->addr.vioserial.port, &b) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u does not have port %u"), + info->addr.vioserial.controller, + info->addr.vioserial.port); + goto cleanup; + } + + if (b) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial port %u on controller %u is already occupied"), + info->addr.vioserial.port, + info->addr.vioserial.controller); + goto cleanup; + } + + ignore_value(virBitmapSetBit(map, info->addr.vioserial.port)); + + ret = 0; + + cleanup: + VIR_FREE(str); + return ret; +} + +/* virDomainVirtioSerialAddrRelease + * + * Release the virtio serial address of the device + */ +int +virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info) +{ + virBitmapPtr map; + char *str = NULL; + int ret = -1; + ssize_t i; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL || + info->addr.vioserial.port == 0) + return 0; + + VIR_DEBUG("Releasing virtio serial %u %u", info->addr.vioserial.controller, + info->addr.vioserial.port); + + i = virDomainVirtioSerialAddrFindController(addrs, info->addr.vioserial.controller); + if (i < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u is missing"), + info->addr.vioserial.controller); + goto cleanup; + } + + map = addrs->controllers[i]->ports; + if (virBitmapClearBit(map, info->addr.vioserial.port) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("virtio serial controller %u does not have port %u"), + info->addr.vioserial.controller, + info->addr.vioserial.port); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(str); + return ret; +} diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 2c3468e..846bf5c 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -173,4 +173,60 @@ int virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs, virDomainDeviceInfoPtr dev) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); virDomainCCWAddressSetPtr virDomainCCWAddressSetCreate(void); + +struct _virDomainVirtioSerialController { + unsigned int idx; + virBitmapPtr ports; +}; + +typedef struct _virDomainVirtioSerialController virDomainVirtioSerialController; +typedef virDomainVirtioSerialController *virDomainVirtioSerialControllerPtr; + +struct _virDomainVirtioSerialAddrSet { + virDomainVirtioSerialControllerPtr *controllers; + size_t ncontrollers; +}; +typedef struct _virDomainVirtioSerialAddrSet virDomainVirtioSerialAddrSet; +typedef virDomainVirtioSerialAddrSet *virDomainVirtioSerialAddrSetPtr; + +virDomainVirtioSerialAddrSetPtr +virDomainVirtioSerialAddrSetCreate(void); +int +virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr addrs, + virDomainControllerDefPtr cont) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDefPtr def) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void +virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs); +int +virDomainVirtioSerialAddrAutoAssign(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info, + bool allowZero) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int +virDomainVirtioSerialAddrAssign(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info, + bool allowZero) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +int +virDomainVirtioSerialAddrReserve(virDomainDefPtr def, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info, + void *data) + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + +int +virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs, + virDomainDeviceInfoPtr info) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __DOMAIN_ADDR_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1fb42ac..f600838 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -108,6 +108,15 @@ virDomainPCIAddressSetFree; virDomainPCIAddressSetGrow; virDomainPCIAddressSlotInUse; virDomainPCIAddressValidate; +virDomainVirtioSerialAddrAssign; +virDomainVirtioSerialAddrAutoAssign; +virDomainVirtioSerialAddrRelease; +virDomainVirtioSerialAddrReserve; +virDomainVirtioSerialAddrSetAddController; +virDomainVirtioSerialAddrSetAddControllers; +virDomainVirtioSerialAddrSetCreate; +virDomainVirtioSerialAddrSetFree; +virDomainVirtioSerialAddrSetRemoveController; # conf/domain_audit.h -- 2.0.5

On Mon, Mar 23, 2015 at 05:46:19PM -0400, John Ferlan wrote: None of the callers worry about that for this function. For the other function, I like sticking to the existing convention: *Free routines usually take a copy of the address, just like virBitmapFree below. I think virFuncFree(foo->ptr); looks more tidy than virFuncFree(&(foo->ptr)); And in most of the cases, foo gets freed anyway. For user-specified controllers, duplicate indexes are rejected by virDomainDefRejectDuplicateControllers, so adding a controller with a non-unique index would be a bug in the auto-allocation logic. I'll squash this in: diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index d9d01fc..cda9ad2 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -754,7 +754,14 @@ virDomainVirtioSerialAddrPlaceController(virDomainVirtioSerialAddrSetPtr addrs, size_t i; for (i = 0; i < addrs->ncontrollers; i++) { - if (addrs->controllers[i]->idx >= cont->idx) + if (addrs->controllers[i]->idx == cont->idx) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("virtio serial controller with index %u already exists" + " in the address set"), + cont->idx); + return -2; + } + if (addrs->controllers[i]->idx > cont->idx) return i; } return -1; @@ -804,7 +811,8 @@ virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs, goto cleanup; cnt->idx = cont->idx; - insertAt = virDomainVirtioSerialAddrPlaceController(addrs, cnt); + if ((insertAt = virDomainVirtioSerialAddrPlaceController(addrs, cnt)) < -1) + goto cleanup; if (VIR_INSERT_ELEMENT(addrs->controllers, insertAt, addrs->ncontrollers, cnt) < 0) goto cleanup; There's nothing to be leaked. Coverity only whines if some callers check the return value and some don't. I'll change the return type to void. No, it's the caller's responsibility not to end up in an endless loop by doing the same thing over and over again. Jan

On 03/17/2015 07:41 AM, Ján Tomko wrote: Coverity determines addrs != NULL, but There's a check here where the "else" is DEADCODE Mostly for my edification... These examples previously although indicating they were "auto-assign" (of sorts) really weren't - they were more force-assign for each example. The way to auto-assign is by setting port='0', correct? However, I'm still missing something from the *auto.args output. It seems the controller='#' is ignored and I guess I don't understand that... Sure "port='0'" (meaning first available port on the controller), but I would have expected to see : kvm.port.0 use "virtio.serial.0.0,nr=1..." (which it does) kvm.port.foo use "virtio.serial.1.0,nr=1..." (on controller 0?, but XML has controller='1') kvm.port.bar use "virtio.serial.1.0,nr=3..." (which it does) kvm.port.wizz use "virtio.serial.0.0,nr=2" (incorrect due to others) kvm.port.ooh use "virtio.serial.1.0,nr=2", (on controller 0?, but XML has controller='1' kvm.port.lla use "virtio.serial.2.0,nr=1" (on controller 0?, but XML has controller='2') Now if been if "lla" used controller='0', then I assume would nr=4 be chosen since 1,2 were auto-assigned, 3 was specifically assigned, thus 4 would be the next one. Continuing with that same logic, the *-autoassign example could have shown that if controller='0',port='2' and 'controller='1',port='1' were preassigned, then the controllers/ports assigned would be 0.0,nr=1, 0.0,nr=3, 0.0,nr=4, 1.0,nr=2 (since only 4 ports on controller='0' can be used w/ 2 be static and controller='1' having port '1' already in use). John

--- src/qemu/qemu_hotplug.c | 21 +++++++++++++++++++-- tests/qemuhotplugtest.c | 2 +- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b2f70d1..3e8d398 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1533,6 +1533,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; char *devstr = NULL; char *charAlias = NULL; + bool need_release = false; + bool allowZero = false; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -1543,6 +1545,16 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) goto cleanup; + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && + chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) + allowZero = true; + + if (virDomainVirtioSerialAddrAutoAssign(priv->vioserialaddrs, + &chr->info, + allowZero) < 0) + goto cleanup; + need_release = true; + if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) goto cleanup; @@ -1574,6 +1586,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, cleanup: if (ret < 0 && virDomainObjIsActive(vm)) qemuDomainChrInsertPreAllocCleanup(vm->def, chr); + if (ret < 0 && need_release) + virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, &chr->info); VIR_FREE(charAlias); VIR_FREE(devstr); return ret; @@ -3970,10 +3984,13 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup; rc = qemuDomainWaitForDeviceRemoval(vm); - if (rc == 0 || rc == 1) + if (rc == 0 || rc == 1) { + virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, &tmpChr->info); ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); - else + } else { ret = 0; + } + cleanup: qemuDomainResetDeviceRemoval(vm); diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 12a7f71..ea2cf77 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -86,7 +86,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, if (event) virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT); - if (qemuDomainAssignPCIAddresses((*vm)->def, priv->qemuCaps, *vm) < 0) + if (qemuDomainAssignAddresses((*vm)->def, priv->qemuCaps, *vm) < 0) goto cleanup; if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps) < 0) -- 2.0.5