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