On Tue, Mar 03, 2015 at 15:44:27 +0100, Ján Tomko wrote:
> Store the available ports of a virtio-serial controller in a virBitmap.
> The bitmaps are stored in a hash table - the controller index
> formatted as a string.
>
> Buses are not tracked, because they aren't supported by QEMU.
> ---
> src/conf/domain_addr.c | 382 +++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_addr.h | 45 ++++++
> src/libvirt_private.syms | 8 +
> 3 files changed, 435 insertions(+)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index fb4a76f..654c95a 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> +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;
> +}
> +
> +void
> +virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs)
> +{
> + if (addrs) {
> + virHashFree(addrs->used);
> + VIR_FREE(addrs);
> + }
> +}
> +
> +/*
> + * Eww, this function compares two unsigned integers stored as a string
> + */
Okay, technically that works as expected, but is it really necessary? If
the user doesn't specify the serial bus to connect the port to, we can
IMO attach it to any of the available ones.
For that approach you can use the virHashSearch function combined with
virBitmapNextClearBit.
Otherwise it would be better to just use an array and not bother with a
bitmap.
> +static int
> +virDomainVirtioSerialAddrCompare(const virHashKeyValuePair *a,
> + const virHashKeyValuePair *b)
> +{
> + const char *key_a = a->key;
> + const char *key_b = b->key;
> +
> + size_t len_a = strlen(key_a);
> + size_t len_b = strlen(key_b);
> +
> + /* with no padding/negative numbers allowed, the longer string
> + * contains a larger number */
> + if (len_a < len_b)
> + return -1;
> + else if (len_a > len_b)
> + return 1;
> + else
> + return strncmp(key_a, key_b, len_a);
> +}
> +
> +static int
> +virDomainVirtioSerialAddrNext(virDomainVirtioSerialAddrSetPtr addrs,
> + virDomainDeviceVirtioSerialAddress *addr,
> + bool allowZero)
> +{
> + virBitmapPtr cur = NULL;
> + char *str = NULL;
> + int ret = -1;
> + virHashKeyValuePairPtr arr = NULL;
> + size_t i, ncontrollers;
> + size_t curidx;
> + ssize_t port, start = 0;
> + unsigned int controller;
> +
> + /* port number 0 is reserved for virtconsoles */
> + if (allowZero)
> + start = -1;
> +
> + /* What controller was the last assigned address on? */
> + if (virAsprintf(&str, "%u", addrs->next.controller) < 0)
> + goto cleanup;
Also making sure that the last controller is always used doens't make
that much sense IMO.
> +
> + if (!(cur = virHashLookup(addrs->used, str))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("The last used virtio serial controller is missing "
> + "from the address set hash table"));
> + goto cleanup;
> + }
Nor finding it problematic if it's missing.
> +
> + /* Look for a free port on the current controller */
> + if ((port = virBitmapNextClearBit(cur, start + addrs->next.port)) >= 0) {
> + controller = addrs->next.controller;
> + goto success;
> + }
> +
> + ncontrollers = virHashSize(addrs->used);
> + arr = virHashGetItems(addrs->used, virDomainVirtioSerialAddrCompare);
> + if (!arr) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to get the hash table as an array"));
> + goto cleanup;
> + }
> +
> + /* Find its position in the hash "array" */
> + for (i = 0; i < ncontrollers; i++) {
> + if (arr[i].value == cur) {
> + curidx = i;
> + break;
> + }
> + }
> + if (i == ncontrollers) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("The last used virtio serial controller is missing from the set"));
> + goto cleanup;
> + }
> +
> + /* Search for a free port after the current controller */
> + for (i = curidx; i < ncontrollers; i++) {
> + cur = (virBitmapPtr) arr[i].value;
> + if ((port = virBitmapNextClearBit(cur, start)) >= 0) {
> + if (virStrToLong_ui(arr[i].key, NULL, 10, &controller) < 0)
> + goto cleanup;
> + goto success;
> + }
> + }
> +
> + for (i = 0; i < curidx; i++) {
> + cur = (virBitmapPtr) arr[i].value;
> + if ((port = virBitmapNextClearBit(cur, start)) >= 0) {
> + if (virStrToLong_ui(arr[i].key, NULL, 10, &controller) < 0)
> + goto cleanup;
> + goto success;
> + }
> + }
Or doing any of this. Finding the first available port is sufficient
IMO.
> +
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Unable to find a free virtio-serial port"));
> +
> + cleanup:
> + VIR_FREE(arr);
> + VIR_FREE(str);
> + 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;
> +}
I think the logic can be simplified by a great extent by not caring
which controller to add the port to if the user didn't specify it
explictly.
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list