Re: [PATCH 1/2 for 8.0] Add the port allocation logic for isa-serial devices.

On 1/13/22 08:33, Divya Garg wrote: > This commit takes care of following cases: > -> Check availability of requested ports. > ->The total number of requested ports should not be more than > VIR_MAX_ISA_SERIAL_PORTS. > ->The ports requested should be less than VIR_MAX_ISA_SERIAL_PORTS. > ->VIR_MAX_ISA_SERIAL_PORTS should correspond to MAX_ISA_SERIAL_PORTS > specified in qemu code commit def337ffda34d331404bd7f1a42726b71500df22. > -> Prevent duplicate device assignments to the same port. > -> In case no ports are provided in the XML, this patch scans the list of unused > isa-serial indices to automatically assign available ports for this VM. > > Signed-off-by: Divya Garg <divya.garg(a)nutanix.com&gt; > --- > src/conf/domain_conf.c | 61 ++++++++++++++++--- > src/conf/domain_conf.h | 6 ++ > ...g-console-compat-2-live+console-virtio.xml | 4 +- > .../qemuhotplug-console-compat-2-live.xml | 4 +- > .../serial-tcp-tlsx509-chardev-notls.xml | 2 +- > .../serial-tcp-tlsx509-chardev-verify.xml | 2 +- > .../serial-tcp-tlsx509-chardev.xml | 2 +- > .../serial-tcp-tlsx509-secret-chardev.xml | 2 +- > .../serial-tcp-tlsx509-chardev.xml | 2 +- > 9 files changed, 68 insertions(+), 17 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 5691b8d2d5..e468e98045 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -5330,6 +5330,56 @@ virDomainHostdevDefPostParse(virDomainHostdevDef *dev, > } > Only a few nitpicks. > > +static int > +virDomainChrIsaSerialDefPostParse(virDomainDef *def) > +{ > + size_t i, j; We like to define variables on separate lines, because it then allows for smaller, easier to understand diffs when changing them. Then, a variable should be declared at the lowest level possible, which in case of @j is inside the other for() loop.

> + size_t isa_serial_count = 0; > + bool used_serial_port[VIR_MAX_ISA_SERIAL_PORTS] = {false}; I prefer spaces around curly-braces.

> + > + /* Perform all the required checks. */ > + for (i = 0; i < def->nserials; i++) { > + > + if (def->serials[i]->targetType != > + VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL) Not your fault, we have a code like this, but I find it more readable if the condition is on one line. There is a recommendation on lines not being 80 chars long, but there are few exceptions to the rule (well, recommendation) and improved code readability is one of them.

> + continue; > + > + if (isa_serial_count++ >= VIR_MAX_ISA_SERIAL_PORTS || > + def->serials[i]->target.port >= VIR_MAX_ISA_SERIAL_PORTS) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Maximum supported number of ISA serial ports is '%d'."), VIR_MAX_ISA_SERIAL_PORTS); Here, and ... > + return -1; > + } > + > + if (def->serials[i]->target.port != -1) { > + if (used_serial_port[def->serials[i]->target.port]) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("target port '%d' already allocated."), def->serials[i]->target.port); ... here, however, the lines could be broken after the error message.

> + return -1; > + } > + used_serial_port[def->serials[i]->target.port] = true; > + } > + } > + > + /* Assign the ports to the devices. */ > + for (i = 0; i < def->nserials; i++) { > + > + if (def->serials[i]->targetType != > + VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL || > + def->serials[i]->target.port != -1) > + continue; > + > + for (j = 0; j < VIR_MAX_ISA_SERIAL_PORTS; j++) { > + if (!used_serial_port[j]) { > + def->serials[i]->target.port = j; > + used_serial_port[j] = true; > + break; > + } > + } > + } > + return 0; > +} > + If you'd look around, we like to separate functions with two empty lines. Yes, we still do have plenty of ancient code with one line separation, but for new code we definitely should use two lines. Then again, an exception would be the function would be added around a code that's still using one line.

> static void > virDomainChrDefPostParse(virDomainChrDef *chr, > const virDomainDef *def) > @@ -6197,6 +6247,9 @@ virDomainDefPostParse(virDomainDef *def, > goto cleanup; > } > > + if (virDomainChrIsaSerialDefPostParse(def) < 0) > + return -1; > + > /* iterate the devices */ > ret = virDomainDeviceInfoIterateFlags(def, > virDomainDefPostParseDeviceIterator, > @@ -19929,14 +19982,6 @@ 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; > - } > - chr->target.port = maxport + 1; > - } > def->serials[def->nserials++] = chr; > } > VIR_FREE(nodes); > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 144ba4dd12..a8f41dc8c2 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1187,6 +1187,12 @@ typedef enum { > VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST > } virDomainChrConsoleTargetType; > > +/* > + * The value of VIR_MAX_ISA_SERIAL_PORTS corresponds to MAX_ISA_SERIAL_PORTS > + * set in qemu code base. We prefer upper case spelling of QEMU. And it's been a long time since I've last booted up my machine with ISA, but IIRC it could only have 4 COM ports, so maybe the limit doesn't come from QEMU really but BIOS of that time? What I'm trying to say is, if this is a limit shared across other hypervisors then it can live under src/conf/ but if isn't shared then it has to go under hypervisor specific dir (src/qemu/ in this case). I'm just going to assume the limit is shared and not QEMU specific.

Michal