[RFC 1/1] update index for serial device using taget.port

[Peter Krempa]

On Wed, Nov 24, 2021 at 07:06:06 -0500, divya wrote:

Hi, just a few quick points, I'm not going to analyze what this does too
deeply at this point.

> From: root <root at localhost.localdomain>

Author should be set properly. Also we require a proper commit message
explaining the issue.

Additionally we require that the submitter declares conformance with
the developer certificate of origin:

https://www.libvirt.org/hacking.html#developer-certificate-of-origin

> 
> ---

[...]

> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5cfb2d91eb..edc5e897be 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -65,6 +65,7 @@
>  #include "virutil.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_DOMAIN

Firstly I see many problems with coding style:

> +#define max_available_isa_serial_ports 4

We usually use uppercase macros

>  
>  VIR_LOG_INIT("conf.domain_conf");
>  
> @@ -19649,6 +19650,10 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt,
>      g_autofree xmlNodePtr *nodes = NULL;
>      g_autofree char *tmp = NULL;
>      g_autoptr(virDomainDef) def = NULL;
> +    uint8_t used_serial_port_buffer = 0;

We have virBitmap for such things.

> +    int isa_serial_count = 0;
> +    int next_available_serial_port = 0;
> +    int max_serial_port = -1;
>  
>      if (!(def = virDomainDefNew(xmlopt)))
>          return NULL;
> @@ -19886,16 +19891,67 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt,
>          if (!chr)
>              return NULL;
>  
> -        if (chr->target.port == -1) {
> -            int maxport = -1;
> -            for (j = 0; j < i; j++) {
> -                if (def->serials[j]->target.port > maxport)
> -                    maxport = def->serials[j]->target.port;
> +        def->serials[def->nserials++] = chr;
> +
> +        // Giving precedence to the isa-serial device since
> +        // only limited ports can be used for such devices.

We don't use line comments (//) but block comments everywhere /* */

> +        if (chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL) {
> +            // Taking the isa serial devices to start of the array.
> +            for (j = def->nserials; j > isa_serial_count; j--)
> +                def->serials[j] = def->serials[j-1];
> +            def->serials[isa_serial_count++] = chr;
> +        }
> +
> +        // Maintaining the buffer for first max_available_isa_serial_ports unused ports.
> +        if (chr->target.port != -1 && chr->target.port <= max_available_isa_serial_ports) {
> +            if (used_serial_port_buffer & (1 << chr->target.port)) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                    _("target port [%d] already allocated."),
> +                    chr->target.port);

This is misaligned.

> +                return NULL;
>              }
> -            chr->target.port = maxport + 1;
> +            used_serial_port_buffer |= 1 << chr->target.port;
> +        }
> +
> +        // Update max serial port used.
> +        if (chr->target.port > max_serial_port)
> +            max_serial_port = chr->target.port;
> +    }
> +
> +    // Assign the ports to the devices.
> +    for (i = 0; i < n; i++) {
> +        if (def->serials[i]->target.port != -1) continue;
> +        // Assign one of the unused ports from first max_available_isa_serial_ports ports
> +        // to isa-serial device.
> +        if (def->serials[i]->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL) {
> +
> +            // Search for the next available port.
> +            while (used_serial_port_buffer & (1 << next_available_serial_port) &&
> +                next_available_serial_port <= max_available_isa_serial_ports) {
> +                next_available_serial_port++;
> +            }
> +
> +            // qemu doesn't support more than max_available_isa_serial_ports isa devices.
> +            if (i > max_available_isa_serial_ports ||
> +                next_available_serial_port > max_available_isa_serial_ports) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                    _("Maximum supported number of ISA serial ports is %d."),
> +                    max_available_isa_serial_ports);
> +                return NULL;
> +            }
> +
> +            used_serial_port_buffer |= 1 << next_available_serial_port;
> +            def->serials[i]->target.port = next_available_serial_port;
> +
> +            // Update max serial port used.
> +            if (def->serials[i]->target.port > max_serial_port)
> +                max_serial_port = def->serials[i]->target.port;
> +
> +        } else {
> +            def->serials[i]->target.port = ++max_serial_port;
>          }
> -        def->serials[def->nserials++] = chr;
>      }

In general, none of this code should be in the parser itself. Not even
the existing code which you are changing actually.

We have code which is meant to adjust and fill defaults for devices. For
your case virDomainChrDefPostParse  or virDomainDefPostParseCommon might
be the right place.

Note that until now the assignment code was rather trivial, but you are
adding a massive amount of logic to this so it definitely doesn't belong
to the parser any more.

> +
>      VIR_FREE(nodes);
>  
>      if ((n = virXPathNodeSet("./devices/console", ctxt, &nodes)) < 0) {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 7a185061d8..c8f8a27f30 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10947,11 +10947,21 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def,
>          return NULL;
>      }
>  
> -    if (virJSONValueObjectAdd(&props,
> -                              "s:driver", virDomainChrSerialTargetModelTypeToString(serial->targetModel),
> -                              "s:chardev", chardev,
> -                              "s:id", serial->info.alias,
> -                              NULL) < 0)
> +    if (serial->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL &&
> +                    serial->target.port != -1) {
> +            if (virJSONValueObjectAdd(&props,
> +                                 "s:driver", virDomainChrSerialTargetModelTypeToString(serial->targetModel),
> +                                 "s:chardev", chardev,
> +                                 "s:id", serial->info.alias,
> +                                 "i:index", serial->target.port,

You can use "k:index" which conditionally adds the 'index' field only
when it's 0 or positive ...

> +                                 NULL) < 0)
> +                    return NULL;
> +    }
> +    else if (virJSONValueObjectAdd(&props,
> +                                 "s:driver", virDomainChrSerialTargetModelTypeToString(serial->targetModel),
> +                                 "s:chardev", chardev,
> +                                 "s:id", serial->info.alias,
> +                                 NULL) < 0)

... so that you don't have to have this else branch here.

Also all of this new code seems to be badly misalligned.

>          return NULL;
>  
>      if (qemuBuildDeviceAddressProps(props, def, &serial->info) < 0)
> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
> index 263a92425c..88c9ea0339 100644
> --- a/tests/qemuhotplugtest.c
> +++ b/tests/qemuhotplugtest.c
> @@ -355,7 +355,6 @@ testQemuHotplug(const void *data)
>      if (keep) {
>          test->vm = vm;
>      } else {
> -        virObjectUnref(vm);
>          test->vm = NULL;
>      }
>      virDomainDeviceDefFree(dev);

This is a very questionable hunk. Was this just for testing?