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

Paolo Bonzini pbonzini at redhat.com
Thu Jan 20 13:32:30 UTC 2011


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




More information about the libvir-list mailing list