[libvirt] [PATCH] Add support for multiple serial ports to Xen driver

[Michal Novotny]

On 01/20/2011 02:32 PM, Paolo Bonzini wrote:
> On 01/20/2011 12:58 PM, root wrote:
>> +                if ((list->type != VIR_CONF_STRING) || (list->str == 
>> NULL))
>> +                    goto skipport;
>> +
>
> I believe this should do
>
>         xenXMError(VIR_ERR_INTERNAL_ERROR,
>                    _("config value %s was malformed"), name);
>         goto cleanup;
>
> similar to xenXMConfigGetString.
>
>> +                chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL;
>> +                chr->type = VIR_DOMAIN_CHR_TYPE_PTY;
>> +
>> +                /* Implement device type of serial port when 
>> appropriate */
>> +                if (STRPREFIX(port, "/dev")) {
>> +                    chr->type = VIR_DOMAIN_CHR_TYPE_DEV;
>> +                    chr->target.port = def->nserials;
>> +                    chr->data.file.path = strdup(port);
>> +
>> +                    if (!chr->data.file.path)
>> +                        goto no_memory;
>> +                }
>
> Can you explain what's going on here ("it needs to be this way to fix 
> xyz" is not an explanation; please give the input and output of 
> xenDaemonParseSxprChar).  In particular, these look like preexisting 
> problems handling
>
>    serial = "/dev/ttyS0"
>
> (where the port is not copied to chr->data.file.path).  Is this 
> correct?  If so, please state this explicitly and check that the above 
> works. It seems to me that your patch fixes /dev/ttyS0 in the array 
> case, but not when it is the only serial port.  If this is correct, 
> "chr->data.file.path = strdup(port);" should be moved to 
> xenDaemonParseSxprChar.  In fact there is already code there to do 
> exactly the same strdup for VIR_DOMAIN_CHR_TYPE_PTY.
>
> Even taking the above into consideration, the code is going through 
> some extra hoops (and has a few bugs):
>
> 1) chr->type should already be correct if the port starts with "/dev", 
> but you have just overridden it to VIR_DOMAIN_CHR_TYPE_PTY.
>
> 2) "chr->type = VIR_DOMAIN_CHR_TYPE_PTY" is not necessary.
>
> 3) "chr->target.port = def->nserials;" should be done always, not just 
> when /dev is matched.
>
> In the end the code here should be simply:
>
>                /* Not really necessary (the enum evaluates to 0), but
>                   I prefer to have it for clarity.  */
>                chr->targetType = VIR_DOMAIN_CHR_TARGET_TYPE_SERIAL;
>                chr->target.port = def->nserials;
>
> matching more or less the code you have in the "else" branch below. 
> Anything except the above should go in xenDaemonParseSxprChar.
>
> Also, please add testcases.  There are many cases that should be 
> covered for both XML->sxpr and xm->XML conversion.
>
> I initially thought there was another problem in the code, which is 
> what to do when the <target> subelement is present and ends up 
> defining a third serial port without the first being present.  However 
> I changed my mind since the qemu driver is anyway disregarding the 
> port number as well.  Given this, the patch is already in pretty good 
> shape.
>
> Thanks,
>
> Paolo
>
Well, I'm working on support for serial port configuration as serial = [ 
"/dev/null", "/dev/ttyS0", "/dev/ttyS1" ] to:

1) pass /dev/null to the guest as /dev/ttyS0
2) pass /dev/ttyS0 to the guest as /dev/ttyS1
3) pass /dev/ttyS1 to the guest as /dev/ttyS2

I think this could be good thing to be implemented as well. Also, the 
libvirt XML definition should be:

<serial type='dev'>
<source path='/dev/null'/>
<target port='0'/>
</serial>
<serial type='dev'>
<source path='/dev/ttyS0'/>
<target port='1'/>
</serial>
<serial type='dev'>
<source path='/dev/ttyS1'/>
<target port='2'/>
</serial>

Are you OK with that?

I'm also thinking of omitting the first <serial> block and automatically 
set the path to /dev/null when target.port is not present in the libvirt 
XML definition but it shouldn't be necessary after all. What do you 
think on this one?

Thanks,
Michal

-- 
Michal Novotny<minovotn at redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat