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@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,
}
+static int
+virDomainChrIsaSerialDefPostParse(virDomainDef *def)
+{
+ size_t i, j;
+ size_t isa_serial_count = 0;
+ bool used_serial_port[VIR_MAX_ISA_SERIAL_PORTS] = {false};
+
+ /* 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)
+ 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);
+ 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);
+ 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;
+}
+
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.
+ */
+#define VIR_MAX_ISA_SERIAL_PORTS 4
+
typedef enum {
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE = 0,
VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL,
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-console-compat-2-live+console-virtio.xml b/tests/qemuhotplugtestdomains/qemuhotplug-console-compat-2-live+console-virtio.xml
index 38311b188c..72516555a0 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-console-compat-2-live+console-virtio.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-console-compat-2-live+console-virtio.xml
@@ -74,7 +74,7 @@
<alias name='serial0'/>
</serial>
<serial type='pty'>
- <target type='isa-serial' port='0'>
+ <target type='isa-serial' port='1'>
<model name='isa-serial'/>
</target>
<alias name='serial1'/>
@@ -82,7 +82,7 @@
<serial type='tcp'>
<source mode='bind' host='0.0.0.0' service='2445'/>
<protocol type='raw'/>
- <target type='isa-serial' port='1'>
+ <target type='isa-serial' port='2'>
<model name='isa-serial'/>
</target>
<alias name='serial2'/>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-console-compat-2-live.xml b/tests/qemuhotplugtestdomains/qemuhotplug-console-compat-2-live.xml
index 3fe205430c..6197a2bfe3 100644
--- a/tests/qemuhotplugtestdomains/qemuhotplug-console-compat-2-live.xml
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-console-compat-2-live.xml
@@ -74,7 +74,7 @@
<alias name='serial0'/>
</serial>
<serial type='pty'>
- <target type='isa-serial' port='0'>
+ <target type='isa-serial' port='1'>
<model name='isa-serial'/>
</target>
<alias name='serial1'/>
@@ -82,7 +82,7 @@
<serial type='tcp'>
<source mode='bind' host='0.0.0.0' service='2445'/>
<protocol type='raw'/>
- <target type='isa-serial' port='1'>
+ <target type='isa-serial' port='2'>
<model name='isa-serial'/>
</target>
<alias name='serial2'/>
diff --git a/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev-notls.xml b/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev-notls.xml
index f27b119270..16734d9cce 100644
--- a/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev-notls.xml
+++ b/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev-notls.xml
@@ -37,7 +37,7 @@
<serial type='tcp'>
<source mode='connect' host='127.0.0.1' service='5555' tls='no'/>
<protocol type='raw'/>
- <target type='isa-serial' port='0'>
+ <target type='isa-serial' port='1'>
<model name='isa-serial'/>
</target>
</serial>
diff --git a/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev-verify.xml b/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev-verify.xml
index be58ccc1da..15ce29c67d 100644
--- a/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev-verify.xml
+++ b/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev-verify.xml
@@ -29,7 +29,7 @@
<serial type='tcp'>
<source mode='connect' host='127.0.0.1' service='5555'/>
<protocol type='raw'/>
- <target port='0'/>
+ <target port='1'/>
</serial>
<console type='udp'>
<source mode='bind' host='127.0.0.1' service='1111'/>
diff --git a/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev.xml b/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev.xml
index be58ccc1da..15ce29c67d 100644
--- a/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev.xml
+++ b/tests/qemuxml2argvdata/serial-tcp-tlsx509-chardev.xml
@@ -29,7 +29,7 @@
<serial type='tcp'>
<source mode='connect' host='127.0.0.1' service='5555'/>
<protocol type='raw'/>
- <target port='0'/>
+ <target port='1'/>
</serial>
<console type='udp'>
<source mode='bind' host='127.0.0.1' service='1111'/>
diff --git a/tests/qemuxml2argvdata/serial-tcp-tlsx509-secret-chardev.xml b/tests/qemuxml2argvdata/serial-tcp-tlsx509-secret-chardev.xml
index 8e44dea365..8bb7cbefae 100644
--- a/tests/qemuxml2argvdata/serial-tcp-tlsx509-secret-chardev.xml
+++ b/tests/qemuxml2argvdata/serial-tcp-tlsx509-secret-chardev.xml
@@ -34,7 +34,7 @@
<serial type='tcp'>
<source mode='connect' host='127.0.0.1' service='5555'/>
<protocol type='raw'/>
- <target port='0'/>
+ <target port='1'/>
</serial>
<console type='udp'>
<source mode='bind' host='127.0.0.1' service='1111'/>
diff --git a/tests/qemuxml2xmloutdata/serial-tcp-tlsx509-chardev.xml b/tests/qemuxml2xmloutdata/serial-tcp-tlsx509-chardev.xml
index aefd3513ce..4cdefed63d 100644
--- a/tests/qemuxml2xmloutdata/serial-tcp-tlsx509-chardev.xml
+++ b/tests/qemuxml2xmloutdata/serial-tcp-tlsx509-chardev.xml
@@ -37,7 +37,7 @@
<serial type='tcp'>
<source mode='connect' host='127.0.0.1' service='5555'/>
<protocol type='raw'/>
- <target type='isa-serial' port='0'>
+ <target type='isa-serial' port='1'>
<model name='isa-serial'/>
</target>
</serial>
--
2.25.1
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@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.
> + 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
On Thu, 13 Jan 2022, Michal Prívozník wrote:
> > 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.
Maybe I read this wrong but I interpreted this commit message in QEMU
repo to mean that the limitation is qemu specific :
commit def337ffda34d331404bd7f1a42726b71500df22
Author: Peter Maydell <peter.maydell@linaro.org>
Date: Fri Apr 20 15:52:46 2018 +0100
serial-isa: Use MAX_ISA_SERIAL_PORTS instead of MAX_SERIAL_PORTS
The ISA serial port handling in serial-isa.c imposes a limit
of 4 serial ports. This is because we only know of 4 IO port
and IRQ settings for them, and is unrelated to the generic
MAX_SERIAL_PORTS limit, though they happen to both be set at
4 currently.
Use a new MAX_ISA_SERIAL_PORTS wherever that is the correct
limit to be checking against.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 20180420145249.32435-11-peter.maydell@linaro.org
> > > 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.
Ok I spoke to Peter about the following commit on @qemu-devel. The
underlying limitation is indeed dictated by the hw it is emulating. Fair
enough! I still think though that we should leave the above comment as is
because this validation in libvirt is dependent on what Qemu allows today.
The reference is important IMHO.
>
> Maybe I read this wrong but I interpreted this commit message in QEMU
> repo to mean that the limitation is qemu specific :
>
> commit def337ffda34d331404bd7f1a42726b71500df22
> Author: Peter Maydell <peter.maydell@linaro.org>
> Date: Fri Apr 20 15:52:46 2018 +0100
>
> serial-isa: Use MAX_ISA_SERIAL_PORTS instead of MAX_SERIAL_PORTS
>
> The ISA serial port handling in serial-isa.c imposes a limit
> of 4 serial ports. This is because we only know of 4 IO port
> and IRQ settings for them, and is unrelated to the generic
> MAX_SERIAL_PORTS limit, though they happen to both be set at
> 4 currently.
>
> Use a new MAX_ISA_SERIAL_PORTS wherever that is the correct
> limit to be checking against.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Message-id: 20180420145249.32435-11-peter.maydell@linaro.org
>
>
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@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
>
© 2016 - 2025 Red Hat, Inc.