[libvirt] [PATCH 1/2 v3] Fix virDomainChrDefParseTargetXML() to parse the target port if available

[Michal Novotny]

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