Re: [libvirt] [PATCHv2 2/5] Add functions to track virtio-serial addresses
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 ""
> and " 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 ||
> > +i
Re: [libvirt] [PATCHv2 2/5] Add functions to track virtio-serial addresses
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...
> 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)
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?
> +{
> +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 ""
and " be rejected elsewhere?
I would think "index" would be unique but this algorithm seems to be
fine and happy with it.
> +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 >=
[libvirt] [PATCHv2 2/5] Add functions to track virtio-serial addresses
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
