4:42 a.m.
On Mon, Mar 23, 2015 at 05:46:19PM -0400, John Ferlan wrote:
On 03/17/2015 07:41 AM, Ján Tomko wrote:
> 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(+)
>
I assumed the ACK to 1/5 sticks...
> +
> +static void
> +virDomainVirtioSerialControllerFree(virDomainVirtioSerialControllerPtr cont)
Should the Free routine take a pointer so that when we VIR_FREE the
pointer the caller doesn't have to "worry" about setting their copy to
NULL?
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.
> +{
> + 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;
> + }
Would entries "<controller type='virtio-serial' index='1'
ports='4'>"
and "<controller type='virtio-serial' index='1'"> be
rejected elsewhere?
I would think "index" would be unique but this algorithm seems to be
fine and happy with it.
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;
> +/* 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;
> +
If 'pos' < 0, we return 0 (and perhaps leak something). OTOH, the
controller was never added and the caller never checks status, maybe
this should just be void (wonder why Coverity didn't whine)...
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.
> +
> +/* 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;
> + }
Should we info->addr.vioserial.port = 0 here to ensure someone doesn't
end up in some loop retrying the same port?
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