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 Novotny<minovotn(a)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.
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(a)redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat