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 Novotny<minovotn@xxxxxxxxxx>
---
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.
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 Novotny<minovotn@xxxxxxxxxx>, RHCE
Virtualization Team (xen userspace), Red Hat
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list