Issue
-----
-----
The port being provided in the xml file of the domain is not getting reflected
in the generated qemu command

For instance, 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

So that qemu understands which port needs to be allocated for the device.

Patch
-----
-----
Out already for the correction :
https://listman.redhat.com/archives/libvir-list/2018-April/msg02302.html

This patch was not followed up. According to me there can be multiple reasons.
I have mentioned them below:
---------------------------------------------------------------------------
---------------------------------------------------------------------------
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.
https://github.com/qemu/qemu/blob/master/hw/char/serial-isa.c#L62

Issue 1:
-------
If we want two isa-serial devices and for the first one if we mention the port
to be 3, then for the next device it automatically assigns 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.

This patch-set tends to solve the problem.
It contains two commits that deals with the issue in following order :
    - check the input provided, and make sure to correctly assign ports
    - provide the corrected port values to qemu

Divya Garg (2):
  Add the port allocation logic for isa-serial devices.
  qemu: add index for isa-serial device using target.port

 src/conf/domain_conf.c                        | 61 ++++++++++++++++---
 src/conf/domain_conf.h                        |  6 ++
 src/qemu/qemu_command.c                       | 25 ++++++--
 ...g-console-compat-2-live+console-virtio.xml |  4 +-
 .../qemuhotplug-console-compat-2-live.xml     |  4 +-
 tests/qemuxml2argvdata/bios.args              |  2 +-
 .../qemuxml2argvdata/console-compat-auto.args |  2 +-
 .../console-compat-auto.x86_64-latest.args    |  2 +-
 .../console-compat-chardev.args               |  2 +-
 .../console-compat-chardev.x86_64-latest.args |  2 +-
 tests/qemuxml2argvdata/console-compat.args    |  2 +-
 .../console-compat.x86_64-latest.args         |  2 +-
 .../qemuxml2argvdata/console-virtio-many.args |  2 +-
 tests/qemuxml2argvdata/controller-order.args  |  2 +-
 .../name-escape.x86_64-2.11.0.args            |  4 +-
 .../name-escape.x86_64-latest.args            |  4 +-
 .../q35-virt-manager-basic.args               |  2 +-
 .../serial-dev-chardev-iobase.args            |  2 +-
 ...rial-dev-chardev-iobase.x86_64-latest.args |  2 +-
 .../qemuxml2argvdata/serial-dev-chardev.args  |  2 +-
 .../serial-dev-chardev.x86_64-latest.args     |  2 +-
 .../qemuxml2argvdata/serial-file-chardev.args |  2 +-
 .../serial-file-chardev.x86_64-latest.args    |  2 +-
 tests/qemuxml2argvdata/serial-file-log.args   |  2 +-
 .../serial-file-log.x86_64-latest.args        |  2 +-
 .../qemuxml2argvdata/serial-many-chardev.args |  4 +-
 .../serial-many-chardev.x86_64-latest.args    |  4 +-
 .../qemuxml2argvdata/serial-pty-chardev.args  |  2 +-
 .../serial-pty-chardev.x86_64-latest.args     |  2 +-
 tests/qemuxml2argvdata/serial-spiceport.args  |  2 +-
 .../serial-spiceport.x86_64-latest.args       |  2 +-
 .../qemuxml2argvdata/serial-tcp-chardev.args  |  2 +-
 .../serial-tcp-chardev.x86_64-latest.args     |  2 +-
 .../serial-tcp-telnet-chardev.args            |  2 +-
 ...rial-tcp-telnet-chardev.x86_64-latest.args |  2 +-
 .../serial-tcp-tlsx509-chardev-notls.args     |  4 +-
 ...p-tlsx509-chardev-notls.x86_64-latest.args |  4 +-
 .../serial-tcp-tlsx509-chardev-notls.xml      |  2 +-
 .../serial-tcp-tlsx509-chardev-verify.args    |  4 +-
 ...-tlsx509-chardev-verify.x86_64-latest.args |  4 +-
 .../serial-tcp-tlsx509-chardev-verify.xml     |  2 +-
 .../serial-tcp-tlsx509-chardev.args           |  4 +-
 ...ial-tcp-tlsx509-chardev.x86_64-latest.args |  4 +-
 .../serial-tcp-tlsx509-chardev.xml            |  2 +-
 .../serial-tcp-tlsx509-secret-chardev.args    |  4 +-
 ...-tlsx509-secret-chardev.x86_64-latest.args |  4 +-
 .../serial-tcp-tlsx509-secret-chardev.xml     |  2 +-
 .../qemuxml2argvdata/serial-udp-chardev.args  |  4 +-
 .../serial-udp-chardev.x86_64-latest.args     |  4 +-
 .../qemuxml2argvdata/serial-unix-chardev.args |  4 +-
 .../serial-unix-chardev.x86_64-latest.args    |  4 +-
 tests/qemuxml2argvdata/serial-vc-chardev.args |  2 +-
 .../serial-vc-chardev.x86_64-latest.args      |  2 +-
 tests/qemuxml2argvdata/user-aliases.args      |  4 +-
 .../virtio-9p-createmode.x86_64-latest.args   |  2 +-
 .../virtio-9p-multidevs.x86_64-latest.args    |  2 +-
 .../x86_64-pc-graphics.x86_64-latest.args     |  2 +-
 .../x86_64-pc-headless.x86_64-latest.args     |  2 +-
 .../x86_64-q35-graphics.x86_64-latest.args    |  2 +-
 .../x86_64-q35-headless.x86_64-latest.args    |  2 +-
 .../serial-tcp-tlsx509-chardev.xml            |  2 +-
 61 files changed, 156 insertions(+), 90 deletions(-)

-- 
2.25.1

On 1/13/22 08:33, Divya Garg wrote:
> Issue

> 
> Divya Garg (2):
>   Add the port allocation logic for isa-serial devices.
>   qemu: add index for isa-serial device using target.port


>  61 files changed, 156 insertions(+), 90 deletions(-)
> 

Hey, couple of points. Usually, when sending new version we do so in a
new thread. And we don't CC random people. Us, developers, are
subscribed to the list.

Anyway, I'm raising only very basic nitpicks with your patches. I'll
squash in the fixes I'm suggesting before pushing. Speaking of which, we
are currently in freeze, preparing for tomorrow's release. so I'll push
these tomorrow.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

On 1/13/22 15:43, Michal Prívozník wrote:
> On 1/13/22 08:33, Divya Garg wrote:
>> Issue
> 
>>
>> Divya Garg (2):
>>   Add the port allocation logic for isa-serial devices.
>>   qemu: add index for isa-serial device using target.port
> 
> 
>>  61 files changed, 156 insertions(+), 90 deletions(-)
>>
> 
> Hey, couple of points. Usually, when sending new version we do so in a
> new thread. And we don't CC random people. Us, developers, are
> subscribed to the list.
> 
> Anyway, I'm raising only very basic nitpicks with your patches. I'll
> squash in the fixes I'm suggesting before pushing. Speaking of which, we
> are currently in freeze, preparing for tomorrow's release. so I'll push
> these tomorrow.
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 

This is now pushed. Congratulations on your first libvirt contribution!

Michal

On 14/01/22 8:17 pm, Michal Prívozník wrote:
> On 1/13/22 15:43, Michal Prívozník wrote:
>> On 1/13/22 08:33, Divya Garg wrote:
>>> Issue
>>> Divya Garg (2):
>>>    Add the port allocation logic for isa-serial devices.
>>>    qemu: add index for isa-serial device using target.port
>>
>>>   61 files changed, 156 insertions(+), 90 deletions(-)
>>>
>> Hey, couple of points. Usually, when sending new version we do so in a
>> new thread. And we don't CC random people. Us, developers, are
>> subscribed to the list.
>>
>> Anyway, I'm raising only very basic nitpicks with your patches. I'll
>> squash in the fixes I'm suggesting before pushing. Speaking of which, we
>> are currently in freeze, preparing for tomorrow's release. so I'll push
>> these tomorrow.
>>
>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>>
> This is now pushed. Congratulations on your first libvirt contribution!
>
> Michal
Thankyou Michal !!
>

On 13/01/22 8:13 pm, Michal Prívozník wrote:
> On 1/13/22 08:33, Divya Garg wrote:
>> Issue
>> Divya Garg (2):
>>    Add the port allocation logic for isa-serial devices.
>>    qemu: add index for isa-serial device using target.port
>
>>   61 files changed, 156 insertions(+), 90 deletions(-)
>>
> Hey, couple of points. Usually, when sending new version we do so in a
> new thread. And we don't CC random people. Us, developers, are
> subscribed to the list.
Hi Michal ! Actually I was told to CC all those who commented on the patch
atleast once ! Hence added them in CC. But in future I will take care of 
this.
Thankyou !!
>
> Anyway, I'm raising only very basic nitpicks with your patches. I'll
> squash in the fixes I'm suggesting before pushing. Speaking of which, we
> are currently in freeze, preparing for tomorrow's release. so I'll push
> these tomorrow.
Thankyou so much !!
>
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>
> Michal
>

On Thu, Jan 13, 2022 at 20:13 Michal Prívozník <mprivozn@redhat.com> wrote:

> On 1/13/22 08:33, Divya Garg wrote:
> > Speaking of which, we
> are currently in freeze, preparing for tomorrow's release. so I'll push
> these tomorrow.



Shouldn’t the bug fix patch have been part of tomorrows release? I thought
bug fixes can be merged even when there is a code freeze. Just trying to
understand the process here.

>
>
>

On Thu, Jan 13, 2022 at 21:27:24 +0530, Ani Sinha wrote:
> On Thu, Jan 13, 2022 at 20:13 Michal Prívozník <mprivozn@redhat.com> wrote:
> 
> > On 1/13/22 08:33, Divya Garg wrote:
> > > Speaking of which, we
> > are currently in freeze, preparing for tomorrow's release. so I'll push
> > these tomorrow.
> 
> 
> 
> Shouldn’t the bug fix patch have been part of tomorrows release? I thought
> bug fixes can be merged even when there is a code freeze. Just trying to
> understand the process here.

No, this change is too invasive IMO and Michal seems to think the same.
The boundaries are not exact and it's up to the person
commiting/approving the code to make the ultimative decision. But the
gist is that you don't push into a RC something which might break the
release.

On 13/01/22 9:41 pm, Peter Krempa wrote:
> On Thu, Jan 13, 2022 at 21:27:24 +0530, Ani Sinha wrote:
>> On Thu, Jan 13, 2022 at 20:13 Michal Prívozník<mprivozn@redhat.com>  wrote:
>>
>>> On 1/13/22 08:33, Divya Garg wrote:
>>>> Speaking of which, we
>>> are currently in freeze, preparing for tomorrow's release. so I'll push
>>> these tomorrow.
>>
>>
>> Shouldn’t the bug fix patch have been part of tomorrows release? I thought
>> bug fixes can be merged even when there is a code freeze. Just trying to
>> understand the process here.
> No, this change is too invasive IMO and Michal seems to think the same.
> The boundaries are not exact and it's up to the person
> commiting/approving the code to make the ultimative decision. But the
> gist is that you don't push into a RC something which might break the
> release.
Hi ! Thankyou so much. So will you like me to post a new iteration, since
today has a code freeze anyway ? Or will it be okay to merge this change ?
>

On Thu, Jan 13, 2022 at 10:30 PM Divya Garg <divya.garg@nutanix.com> wrote:

>
> On 13/01/22 9:41 pm, Peter Krempa wrote:
>
> On Thu, Jan 13, 2022 at 21:27:24 +0530, Ani Sinha wrote:
>
> On Thu, Jan 13, 2022 at 20:13 Michal Prívozník <mprivozn@redhat.com> <mprivozn@redhat.com> wrote:
>
>
> On 1/13/22 08:33, Divya Garg wrote:
>
> Speaking of which, we
>
> are currently in freeze, preparing for tomorrow's release. so I'll push
> these tomorrow.
>
>
>
> Shouldn’t the bug fix patch have been part of tomorrows release? I thought
> bug fixes can be merged even when there is a code freeze. Just trying to
> understand the process here.
>
> No, this change is too invasive IMO and Michal seems to think the same.
> The boundaries are not exact and it's up to the person
> commiting/approving the code to make the ultimative decision. But the
> gist is that you don't push into a RC something which might break the
> release.
>
> Hi ! Thankyou so much. So will you like me to post a new iteration, since
> today has a code freeze anyway ? Or will it be okay to merge this change ?
>


Michal has already said he will push your change after the release
tomorrow. At the same time he will fix things he was suggesting. So you can
relax and wait for him to push. No need to churn out a new iteration.