<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> On 24/11/21 6:21 pm, Peter Krempa wrote: <blockquote type="cite" cite="mid:YZ41SB1VPDz7lwjX@angien.pipo.sk"> <pre class="moz-quote-pre" wrap="">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. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">From: root <a class="moz-txt-link-rfc2396E" href="mailto:root@localhost.localdomain"><root@localhost.localdomain></a> </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> 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: <a class="moz-txt-link-freetext" 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> </pre> </blockquote> This patch was just for getting the understanding of historical reasons of the port allocation logic we process today and if the changes suggested looks good and doesn't violate any convention or if we are planning to have a different handling for such cases. <blockquote type="cite" cite="mid:YZ41SB1VPDz7lwjX@angien.pipo.sk"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> --- </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> [...] </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> 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 </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Firstly I see many problems with coding style:</pre> </blockquote> I am really sorry for that, I didnt give much attention to that. It was just to show how I will be changing the port allocation logic for isa-serial devices. <blockquote type="cite" cite="mid:YZ41SB1VPDz7lwjX@angien.pipo.sk"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+#define max_available_isa_serial_ports 4 </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> We usually use uppercase macros </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> 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; </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> We have virBitmap for such things.</pre> </blockquote> Noted. Thanks. <blockquote type="cite" cite="mid:YZ41SB1VPDz7lwjX@angien.pipo.sk"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ 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. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> We don't use line comments (//) but block comments everywhere /* */</pre> </blockquote> Noted <blockquote type="cite" cite="mid:YZ41SB1VPDz7lwjX@angien.pipo.sk"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ 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); </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> This is misaligned.</pre> </blockquote> Noted <blockquote type="cite" cite="mid:YZ41SB1VPDz7lwjX@angien.pipo.sk"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ 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; } </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> 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.</pre> </blockquote> Earlier we never checked for repeated ports or the max port that can be assigned to a particular design. Besides we used to assign the ports blindly without considering what kind of device it is. Was there any specefic reason for this ? <blockquote type="cite" cite="mid:YZ41SB1VPDz7lwjX@angien.pipo.sk"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ 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, </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> You can use "k:index" which conditionally adds the 'index' field only when it's 0 or positive ... </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ NULL) < 0) + return NULL; + } + else if (virJSONValueObjectAdd(&props, + "s:driver", virDomainChrSerialTargetModelTypeToString(serial->targetModel), + "s:chardev", chardev, + "s:id", serial->info.alias, + NULL) < 0) </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> ... so that you don't have to have this else branch here.</pre> </blockquote> Thanks, but this will be anyhow required to check if the device is ISA-serial or not. <blockquote type="cite" cite="mid:YZ41SB1VPDz7lwjX@angien.pipo.sk"> <pre class="moz-quote-pre" wrap=""> Also all of this new code seems to be badly misalligned.</pre> </blockquote> Will update the allignment. <blockquote type="cite" cite="mid:YZ41SB1VPDz7lwjX@angien.pipo.sk"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> 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); </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> This is a very questionable hunk. Was this just for testing?</pre> </blockquote> Hi Peter. This was an RFC and yeah, still not finalised. I will be working on formatting and restructure the code once the logic goods. Before putting more efforts in finalising the code I wanted to confirm if the changes in the port allocation logic for isa-serial devices looks good or not. Sharing the details : Issue ----- The port being provided in the xml file of the domain is not being used for the creation of qemu command. On adding the serial device : <serial> <target type='serial' port='3'/> </serial> Generated qemu command will look like : /usr/libexec/qemu-kvm ...\ -device isa-serial,chardev=charserial0,id=serial0 Actually it should be : /usr/libexec/qemu-kvm ...\ -device isa-serial,chardev=charserial0,id=serial0,index=3 There is a patch out there already for the correction : <a class="moz-txt-link-freetext" href="https://listman.redhat.com/archives/libvir-list/2018-April/msg02302.html">https://listman.redhat.com/archives/libvir-list/2018-April/msg02302.html</a> But This patch was not followed up. According to me there are multiple reasons Reasons for not following up ---------------------------- ---------------------------- Index : specifies the index number of a connector port. If not specified, the index is automatically incremented. This logic exists both on qemu as well as libvirt. <a class="moz-txt-link-freetext" href="https://github.com/qemu/qemu/blob/master/hw/char/serial-isa.c#L62">https://github.com/qemu/qemu/blob/master/hw/char/serial-isa.c#L62</a> Issue 1: If we want two isa-serial devices and for the first one is we mention the port to be 3, then for the next device it not automatically assign the port number 4 which will throw the following error : error: internal error: process exited while connecting to monitor: 2021-11-12T11:05:31.169987Z qemu-kvm: -device isa-serial,chardev=charserial2,id=serial2,index=5: Max. supported number of ISA serial ports is 4. But we are left with 3 ports (0,1,2) which are unused. So ideally we should have used them. Issue 2: It is possible that two devices get the same port address which might lead to a lot of ambiguity. Example: we want two devices and for the second one we provide the index 0. Then from default logic the first device will be allotted port 0 and the second device will overwrite it and get port 0. Solution : ---------- ---------- Port allocation logic 1. Precedence should be given to serial devices as we only have the first 4 ports for them. 1.1. Check the command line/xml file, scan for all the devices mentioned and then start with the isa-serial devices for port allocation. 2.Maintain a buffer(bitmap) for marking the allocated ports. 3.While assigning a port to the device 3.1. If no port is provided by the user : provide the next available port. 3.2. Else check: 3.2.1. If the port is already allocated : throw the error. 3.2.2. Else allocate the port. 3.3. If out of ports : throw error -> qemu throws the error. Libvirt also manages the port numbers with the auto increment logic but never updates the index value using the port number. Hence index should be assigned with proper port allocation logic. <blockquote type="cite" cite="mid:YZ41SB1VPDz7lwjX@angien.pipo.sk"> <pre class="moz-quote-pre" wrap=""> </pre> </blockquote> </body> </html>