[PATCH 1/2 for 8.0] Add the port allocation logic for isa-serial devices.
Divya Garg
divya.garg at nutanix.com
Fri Jan 14 18:32:08 UTC 2022
Thanks Michal !! For reviewing and merging my patch. I will keep all the
nits
pointed by you in mind for my next patches. :)
On 13/01/22 8:50 pm, Michal Prívozník wrote:
> 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 at nutanix.com>
>> ---
>> 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.
Ohh yeah right !! I will keep these details in mind for my next patch.
>
>> + size_t isa_serial_count = 0;
>> + bool used_serial_port[VIR_MAX_ISA_SERIAL_PORTS] = {false};
> I prefer spaces around curly-braces.
Sure. Actually all the space related issue was coming up with the tests.
Hence I thought it will be okay not to add spaces.
>
>> +
>> + /* 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.
Noted
>
>> + 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.
In the coding convention it was written not to break the error messages
hence followed that rule. But next time will break after 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.
Noted.
>
>> 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.
Thanks Michal ! Should I run some experiments to understand this ? Or
are we good for now ?
>
> Michal
>
More information about the libvir-list
mailing list