<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style>
</head>
<body lang="EN-IN" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span style="mso-fareast-language:EN-US">Hi Peter,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="mso-fareast-language:EN-US">This code was just for test and reference. I primarily wanted to get review
<o:p></o:p></span></p>
<p class="MsoNormal"><span style="mso-fareast-language:EN-US">on RFC. I have sent the cover letter explaining the issues as well as the
<o:p></o:p></span></p>
<p class="MsoNormal"><span style="mso-fareast-language:EN-US">change. </span><span style="color:black">This patch is still in testing phase. Before moving forward I wanted
<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black">to confirm if my changes and the logic for port allocation looks good and<br>
get some history behind the earlier logic.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:black">Attaching the doc explaining the issue and change just for reference.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black">PFA<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:black">Thanks<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black">Regards<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black">Divya Garg<o:p></o:p></span></p>
<p class="MsoNormal"><span style="mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal" style="margin-bottom:12.0pt"><b><span style="font-size:12.0pt;color:black">From:
</span></b><span style="font-size:12.0pt;color:black">Peter Krempa <pkrempa@redhat.com><br>
<b>Date: </b>Wednesday, 24 November 2021 at 6:22 PM<br>
<b>To: </b>Divya Garg <divya.garg@nutanix.com><br>
<b>Cc: </b>libvir-list@redhat.com <libvir-list@redhat.com>, Manish Mishra <manish.mishra@nutanix.com>, Prerna Saxena <prerna.saxena@nutanix.com><br>
<b>Subject: </b>Re: [RFC 1/1] update index for serial device using taget.port<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt">On Wed, Nov 24, 2021 at 07:06:06 -0500, divya wrote:<br>
<br>
Hi, just a few quick points, I'm not going to analyze what this does too<br>
deeply at this point.<br>
<br>
> From: root <root@localhost.localdomain><br>
<br>
Author should be set properly. Also we require a proper commit message<br>
explaining the issue.<br>
<br>
Additionally we require that the submitter declares conformance with<br>
the developer certificate of origin:<br>
<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__www.libvirt.org_hacking.html-23developer-2Dcertificate-2Dof-2Dorigin&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=2QGHz-fTCVWImEBKe1ZcSe5t6UfasnhvdzD5DcixwOE&m=IUWwXppAjhBSqGVOlRTyZ70mhLrTUeeQA692o-Q942j1SPk8i8nrvQPV9KtRHOwl&s=23RVgvzwCkcam2r_OcJ3920vDnxkmIm4tY15Bb4LdFY&e=">https://urldefense.proofpoint.com/v2/url?u=https-3A__www.libvirt.org_hacking.html-23developer-2Dcertificate-2Dof-2Dorigin&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=2QGHz-fTCVWImEBKe1ZcSe5t6UfasnhvdzD5DcixwOE&m=IUWwXppAjhBSqGVOlRTyZ70mhLrTUeeQA692o-Q942j1SPk8i8nrvQPV9KtRHOwl&s=23RVgvzwCkcam2r_OcJ3920vDnxkmIm4tY15Bb4LdFY&e=</a>
<br>
<br>
> <br>
> ---<br>
<br>
[...]<br>
<br>
> <br>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c<br>
> index 5cfb2d91eb..edc5e897be 100644<br>
> --- a/src/conf/domain_conf.c<br>
> +++ b/src/conf/domain_conf.c<br>
> @@ -65,6 +65,7 @@<br>
> #include "virutil.h"<br>
> <br>
> #define VIR_FROM_THIS VIR_FROM_DOMAIN<br>
<br>
Firstly I see many problems with coding style:<br>
<br>
> +#define max_available_isa_serial_ports 4<br>
<br>
We usually use uppercase macros<br>
<br>
> <br>
> VIR_LOG_INIT("conf.domain_conf");<br>
> <br>
> @@ -19649,6 +19650,10 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt,<br>
> g_autofree xmlNodePtr *nodes = NULL;<br>
> g_autofree char *tmp = NULL;<br>
> g_autoptr(virDomainDef) def = NULL;<br>
> + uint8_t used_serial_port_buffer = 0;<br>
<br>
We have virBitmap for such things.<br>
<br>
> + int isa_serial_count = 0;<br>
> + int next_available_serial_port = 0;<br>
> + int max_serial_port = -1;<br>
> <br>
> if (!(def = virDomainDefNew(xmlopt)))<br>
> return NULL;<br>
> @@ -19886,16 +19891,67 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt,<br>
> if (!chr)<br>
> return NULL;<br>
> <br>
> - if (chr->target.port == -1) {<br>
> - int maxport = -1;<br>
> - for (j = 0; j < i; j++) {<br>
> - if (def->serials[j]->target.port > maxport)<br>
> - maxport = def->serials[j]->target.port;<br>
> + def->serials[def->nserials++] = chr;<br>
> +<br>
> + // Giving precedence to the isa-serial device since<br>
> + // only limited ports can be used for such devices.<br>
<br>
We don't use line comments (//) but block comments everywhere /* */<br>
<br>
> + if (chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL) {<br>
> + // Taking the isa serial devices to start of the array.<br>
> + for (j = def->nserials; j > isa_serial_count; j--)<br>
> + def->serials[j] = def->serials[j-1];<br>
> + def->serials[isa_serial_count++] = chr;<br>
> + }<br>
> +<br>
> + // Maintaining the buffer for first max_available_isa_serial_ports unused ports.<br>
> + if (chr->target.port != -1 && chr->target.port <= max_available_isa_serial_ports) {<br>
> + if (used_serial_port_buffer & (1 << chr->target.port)) {<br>
> + virReportError(VIR_ERR_INTERNAL_ERROR,<br>
> + _("target port [%d] already allocated."),<br>
> + chr->target.port);<br>
<br>
This is misaligned.<br>
<br>
> + return NULL;<br>
> }<br>
> - chr->target.port = maxport + 1;<br>
> + used_serial_port_buffer |= 1 << chr->target.port;<br>
> + }<br>
> +<br>
> + // Update max serial port used.<br>
> + if (chr->target.port > max_serial_port)<br>
> + max_serial_port = chr->target.port;<br>
> + }<br>
> +<br>
> + // Assign the ports to the devices.<br>
> + for (i = 0; i < n; i++) {<br>
> + if (def->serials[i]->target.port != -1) continue;<br>
> + // Assign one of the unused ports from first max_available_isa_serial_ports ports<br>
> + // to isa-serial device.<br>
> + if (def->serials[i]->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL) {<br>
> +<br>
> + // Search for the next available port.<br>
> + while (used_serial_port_buffer & (1 << next_available_serial_port) &&<br>
> + next_available_serial_port <= max_available_isa_serial_ports) {<br>
> + next_available_serial_port++;<br>
> + }<br>
> +<br>
> + // qemu doesn't support more than max_available_isa_serial_ports isa devices.<br>
> + if (i > max_available_isa_serial_ports ||<br>
> + next_available_serial_port > max_available_isa_serial_ports) {<br>
> + virReportError(VIR_ERR_INTERNAL_ERROR,<br>
> + _("Maximum supported number of ISA serial ports is %d."),<br>
> + max_available_isa_serial_ports);<br>
> + return NULL;<br>
> + }<br>
> +<br>
> + used_serial_port_buffer |= 1 << next_available_serial_port;<br>
> + def->serials[i]->target.port = next_available_serial_port;<br>
> +<br>
> + // Update max serial port used.<br>
> + if (def->serials[i]->target.port > max_serial_port)<br>
> + max_serial_port = def->serials[i]->target.port;<br>
> +<br>
> + } else {<br>
> + def->serials[i]->target.port = ++max_serial_port;<br>
> }<br>
> - def->serials[def->nserials++] = chr;<br>
> }<br>
<br>
In general, none of this code should be in the parser itself. Not even<br>
the existing code which you are changing actually.<br>
<br>
We have code which is meant to adjust and fill defaults for devices. For<br>
your case virDomainChrDefPostParse or virDomainDefPostParseCommon might<br>
be the right place.<br>
<br>
Note that until now the assignment code was rather trivial, but you are<br>
adding a massive amount of logic to this so it definitely doesn't belong<br>
to the parser any more.<br>
<br>
> +<br>
> VIR_FREE(nodes);<br>
> <br>
> if ((n = virXPathNodeSet("./devices/console", ctxt, &nodes)) < 0) {<br>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c<br>
> index 7a185061d8..c8f8a27f30 100644<br>
> --- a/src/qemu/qemu_command.c<br>
> +++ b/src/qemu/qemu_command.c<br>
> @@ -10947,11 +10947,21 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def,<br>
> return NULL;<br>
> }<br>
> <br>
> - if (virJSONValueObjectAdd(&props,<br>
> - "s:driver", virDomainChrSerialTargetModelTypeToString(serial->targetModel),<br>
> - "s:chardev", chardev,<br>
> - "s:id", serial->info.alias,<br>
> - NULL) < 0)<br>
> + if (serial->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL &&<br>
> + serial->target.port != -1) {<br>
> + if (virJSONValueObjectAdd(&props,<br>
> + "s:driver", virDomainChrSerialTargetModelTypeToString(serial->targetModel),<br>
> + "s:chardev", chardev,<br>
> + "s:id", serial->info.alias,<br>
> + "i:index", serial->target.port,<br>
<br>
You can use "k:index" which conditionally adds the 'index' field only<br>
when it's 0 or positive ...<br>
<br>
> + NULL) < 0)<br>
> + return NULL;<br>
> + }<br>
> + else if (virJSONValueObjectAdd(&props,<br>
> + "s:driver", virDomainChrSerialTargetModelTypeToString(serial->targetModel),<br>
> + "s:chardev", chardev,<br>
> + "s:id", serial->info.alias,<br>
> + NULL) < 0)<br>
<br>
... so that you don't have to have this else branch here.<br>
<br>
Also all of this new code seems to be badly misalligned.<br>
<br>
> return NULL;<br>
> <br>
> if (qemuBuildDeviceAddressProps(props, def, &serial->info) < 0)<br>
> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c<br>
> index 263a92425c..88c9ea0339 100644<br>
> --- a/tests/qemuhotplugtest.c<br>
> +++ b/tests/qemuhotplugtest.c<br>
> @@ -355,7 +355,6 @@ testQemuHotplug(const void *data)<br>
> if (keep) {<br>
> test->vm = vm;<br>
> } else {<br>
> - virObjectUnref(vm);<br>
> test->vm = NULL;<br>
> }<br>
> virDomainDeviceDefFree(dev);<br>
<br>
This is a very questionable hunk. Was this just for testing?<o:p></o:p></p>
</div>
</div>
</body>
</html>