Re: [libvirt] [PATCH 1/2 v3] Fix virDomainChrDefParseTargetXML() to parse the target port if available
On 02/18/2011 05:49 PM, Eric Blake wrote: On 02/18/2011 07:11 AM, Michal Novotny wrote: Hi, this is the patch to fix the virDomainChrDefParseTargetXML() functionality to parse the target port from XML if available. This is necessary for multiple serial port support which is the second part of this patch. Michal Signed-off-by: Michal Novotnyminov...@redhat.com --- src/conf/domain_conf.c |7 +-- 1 files changed, 5 insertions(+), 2 deletions(-) @@ -5566,7 +5568,8 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (!chr) goto error; -chr-target.port = i; +if (chr-target.port 0) +chr-target.port = i; def-serials[def-nserials++] = chr; I think this fails to reject collisions, if twoserial devices request the same port number. Also, I think it mis-handles the case where things are interleaved out of order: devices serial type='dev' source .../ target port='1'/ /serial serial type='dev' source .../ target type='serial'/ serial/ /devices The second serial device should default to the first available port number (0), but it looks like this patch will assign it to port 1 and cause a duplicate. Well, if such definition should be valid then it may present the issue you stated above. Honestly I don't know why there was port attribute in the target node since with the code it was having it's unlikely it was used ever so I guess I should go through all the serial ports to check whether the target port is used or not starting from zero (first position as it's zero based). Like when you have: 1. You have definition like target.port = 1 and another 2 definitions with target.port missing 2. Create first device with target.port = 1 3. Second serial port is missing target.port so start from 0 - this is still free so assign target.port = 0 4. Third serial port is missing target.port, again start from 0 - both 0 and 1 are assigned so use next, i.e. target.port = 2 This way it should be working fine, right? Also, def-parallels shares virDomainDefParseXML, so it probably needs the same treatment. That is, I think this side of the patch still needs a bit of work. Ok, I was thinking that this kind of treatment may be necessary there for the future as well but currently I don't know whether any hypervisor supports multiple parallel ports. But I have to agree it's good to have this fixed the very same time like serial ports handling so I'll fix this as well. We've got other code in domain_conf.c that assigns ports to the first available slot; for example, look near line 5628 at how virtio-serial ports are assigned using maxport and traversal of all previously assigned ports. Oh, thanks. I think that's basically similar to steps I wrote above since it's describing traversal lookup of all previously assigned ports as well but the solution in domain_conf.c may be better :) Also, I think there are no maxports constant identifying maximum number of serial ports to be available for the domain so I'll skip this one probably. I didn't have a look at the code yet so maybe maxports means something else. Michal -- Michal Novotnyminov...@redhat.com, RHCE Virtualization Team (xen userspace), Red Hat -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2 v3] Fix virDomainChrDefParseTargetXML() to parse the target port if available
[snip]
We've got other code in domain_conf.c that assigns ports
to the first available slot; for example, look near line 5628 at how
virtio-serial ports are assigned using maxport and traversal of all
previously assigned ports.
Is the code correct there?
The code is:
if (chr-info.type ==
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL
chr-info.addr.vioserial.port == 0) {
int maxport = 0;
int j;
for (j = 0 ; j i ; j++) {
virDomainChrDefPtr thischr = def-channels[j];
if (thischr-info.type ==
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL
thischr-info.addr.vioserial.controller ==
chr-info.addr.vioserial.controller
thischr-info.addr.vioserial.bus ==
chr-info.addr.vioserial.bus
(int)thischr-info.addr.vioserial.port maxport)
maxport = thischr-info.addr.vioserial.port;
}
chr-info.addr.vioserial.port = maxport + 1;
}
That is if it's found you're having maxport set to maximum value + 1,
that's fine but if it's not found it doesn't start from 0 but it starts
from number 1 since chr-info.addr.vioserial.port = maxport + 1; line.
Maxport is being set to 0 and when nothing is found then the code is
chr-info.addr.vioserial.port = 0 + 1 therefore resulting into
chr-info.addr.vioserial.port = 1;. Based on this it doesn't start
zero-based since this position (port = 0) is unassigned but only next
position (port = 1) is defined.
Is that behavior correct?
Thanks,
Michal
--
Michal Novotnyminov...@redhat.com, RHCE
Virtualization Team (xen userspace), Red Hat
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2 v3] Fix virDomainChrDefParseTargetXML() to parse the target port if available
On 02/21/2011 02:38 AM, Michal Novotny wrote:
[snip]
We've got other code in domain_conf.c that assigns ports
to the first available slot; for example, look near line 5628 at how
virtio-serial ports are assigned using maxport and traversal of all
previously assigned ports.
Is the code correct there?
The code is:
if (chr-info.type ==
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL
chr-info.addr.vioserial.port == 0) {
int maxport = 0;
virtio-serial has to start at port 1 (not 0, which is reserved), so
maxport starts at 0...
chr-info.addr.vioserial.port = maxport + 1;
and the assignment guarantees 1 or greater. But serial and parallel
support port 0, so start maxport at -1 instead of 0, to reuse the same
logic.
--
Eric Blake ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2 v3] Fix virDomainChrDefParseTargetXML() to parse the target port if available
On 02/21/2011 05:01 PM, Eric Blake wrote:
On 02/21/2011 02:38 AM, Michal Novotny wrote:
[snip]
We've got other code in domain_conf.c that assigns ports
to the first available slot; for example, look near line 5628 at how
virtio-serial ports are assigned using maxport and traversal of all
previously assigned ports.
Is the code correct there?
The code is:
if (chr-info.type ==
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL
chr-info.addr.vioserial.port == 0) {
int maxport = 0;
virtio-serial has to start at port 1 (not 0, which is reserved), so
maxport starts at 0...
chr-info.addr.vioserial.port = maxport + 1;
and the assignment guarantees 1 or greater. But serial and parallel
support port 0, so start maxport at -1 instead of 0, to reuse the same
logic.
Oh, ok, I was thinking whether it's a bug in virtio driver or not.
Thanks for clarification it has to start at 1 :)
Thanks,
Michal
--
Michal Novotnyminov...@redhat.com, RHCE
Virtualization Team (xen userspace), Red Hat
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2 v3] Fix virDomainChrDefParseTargetXML() to parse the target port if available
On 02/18/2011 07:11 AM, Michal Novotny wrote: Hi, this is the patch to fix the virDomainChrDefParseTargetXML() functionality to parse the target port from XML if available. This is necessary for multiple serial port support which is the second part of this patch. Michal Signed-off-by: Michal Novotny minov...@redhat.com --- src/conf/domain_conf.c |7 +-- 1 files changed, 5 insertions(+), 2 deletions(-) @@ -5566,7 +5568,8 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (!chr) goto error; -chr-target.port = i; +if (chr-target.port 0) +chr-target.port = i; def-serials[def-nserials++] = chr; I think this fails to reject collisions, if two serial devices request the same port number. Also, I think it mis-handles the case where things are interleaved out of order: devices serial type='dev' source .../ target port='1'/ /serial serial type='dev' source .../ target type='serial'/ serial/ /devices The second serial device should default to the first available port number (0), but it looks like this patch will assign it to port 1 and cause a duplicate. Also, def-parallels shares virDomainDefParseXML, so it probably needs the same treatment. That is, I think this side of the patch still needs a bit of work. We've got other code in domain_conf.c that assigns ports to the first available slot; for example, look near line 5628 at how virtio-serial ports are assigned using maxport and traversal of all previously assigned ports. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
