8:31 a.m.
For some crazy backward compatibility, a console can by just an alias to
a serial device. This is detected in the XML formating function which
takes the values to format from corresponding serial device. Including
the device alias. This results in wrong alias being written into the XML
definition:
<console type='pty' tty='/dev/pts/5'>
...
<alias name='serial0'/>
</console>
While holding the correct alias still in the memory, it doesn't matter.
However, it starts to matter as soon as libvirtd is restarted and the
(incorrect) alias is read from status file.
---
src/conf/domain_conf.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 011de71..61de836 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16417,6 +16417,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
memcpy(&console, def->serials[n], sizeof(console));
console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
console.targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
+ memcpy(&console.info, &def->consoles[n]->info,
sizeof(console.info));
} else {
memcpy(&console, def->consoles[n], sizeof(console));
}
@@ -16427,11 +16428,20 @@ virDomainDefFormatInternal(virDomainDefPtr def,
def->nconsoles == 0 &&
def->nserials > 0) {
virDomainChrDef console;
+ char *alias = NULL;
memcpy(&console, def->serials[n], sizeof(console));
console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
console.targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
- if (virDomainChrDefFormat(buf, &console, flags) < 0)
+ if (console.info.alias) {
+ if (VIR_STRDUP(alias, "console0") < 0)
+ goto error;
+ console.info.alias = alias;
+ }
+ if (virDomainChrDefFormat(buf, &console, flags) < 0) {
+ VIR_FREE(alias);
goto error;
+ }
+ VIR_FREE(alias);
}
for (n = 0; n < def->nchannels; n++)
--
1.8.1.5
8:55 a.m.
On Fri, Jun 28, 2013 at 03:31:17PM +0200, Michal Privoznik wrote:
For some crazy backward compatibility, a console can by just an alias
to
a serial device. This is detected in the XML formating function which
takes the values to format from corresponding serial device. Including
the device alias. This results in wrong alias being written into the XML
definition:
<console type='pty' tty='/dev/pts/5'>
...
<alias name='serial0'/>
</console>
While holding the correct alias still in the memory, it doesn't matter.
However, it starts to matter as soon as libvirtd is restarted and the
(incorrect) alias is read from status file.
---
src/conf/domain_conf.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 011de71..61de836 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16417,6 +16417,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
memcpy(&console, def->serials[n], sizeof(console));
console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
console.targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
+ memcpy(&console.info, &def->consoles[n]->info,
sizeof(console.info));
} else {
memcpy(&console, def->consoles[n], sizeof(console));
}
@@ -16427,11 +16428,20 @@ virDomainDefFormatInternal(virDomainDefPtr def,
def->nconsoles == 0 &&
def->nserials > 0) {
virDomainChrDef console;
+ char *alias = NULL;
memcpy(&console, def->serials[n], sizeof(console));
console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
console.targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
- if (virDomainChrDefFormat(buf, &console, flags) < 0)
+ if (console.info.alias) {
+ if (VIR_STRDUP(alias, "console0") < 0)
+ goto error;
+ console.info.alias = alias;
+ }
+ if (virDomainChrDefFormat(buf, &console, flags) < 0) {
+ VIR_FREE(alias);
goto error;
+ }
+ VIR_FREE(alias);
This isn't any more correct that your previous patch. For the
dummy <console> elements, the alias *must* be copied from /
identical to the corresponding <serial> device config.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:45 a.m.
On 28.06.2013 15:55, Daniel P. Berrange wrote:
On Fri, Jun 28, 2013 at 03:31:17PM +0200, Michal Privoznik wrote:
> For some crazy backward compatibility, a console can by just an alias to
> a serial device. This is detected in the XML formating function which
> takes the values to format from corresponding serial device. Including
> the device alias. This results in wrong alias being written into the XML
> definition:
>
> <console type='pty' tty='/dev/pts/5'>
> ...
> <alias name='serial0'/>
> </console>
>
> While holding the correct alias still in the memory, it doesn't matter.
> However, it starts to matter as soon as libvirtd is restarted and the
> (incorrect) alias is read from status file.
> ---
> src/conf/domain_conf.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 011de71..61de836 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16417,6 +16417,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> memcpy(&console, def->serials[n], sizeof(console));
> console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
> console.targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
> + memcpy(&console.info, &def->consoles[n]->info,
sizeof(console.info));
> } else {
> memcpy(&console, def->consoles[n], sizeof(console));
> }
> @@ -16427,11 +16428,20 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> def->nconsoles == 0 &&
> def->nserials > 0) {
> virDomainChrDef console;
> + char *alias = NULL;
> memcpy(&console, def->serials[n], sizeof(console));
> console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE;
> console.targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
> - if (virDomainChrDefFormat(buf, &console, flags) < 0)
> + if (console.info.alias) {
> + if (VIR_STRDUP(alias, "console0") < 0)
> + goto error;
> + console.info.alias = alias;
> + }
> + if (virDomainChrDefFormat(buf, &console, flags) < 0) {
> + VIR_FREE(alias);
> goto error;
> + }
> + VIR_FREE(alias);
This isn't any more correct that your previous patch. For the
dummy <console> elements, the alias *must* be copied from /
identical to the corresponding <serial> device config.
Daniel
So what are you saying is, it's the <serial> that has the wrong alias
assigned? The output should be like this then?
<serial type='pty'>
<source path='/dev/pts/5'/>
<target type='isa-serial' port='0'/>
<alias name='console0'/>
<!-- originaly was serial0 -->
</serial>
<console type='pty' tty='/dev/pts/5'>
<source path='/dev/pts/5'/>
<target type='serial' port='0'/>
<alias name='console0'/>
</console>
Michal
10:01 a.m.
On Fri, Jun 28, 2013 at 04:45:08PM +0200, Michal Privoznik wrote:
On 28.06.2013 15:55, Daniel P. Berrange wrote:
> On Fri, Jun 28, 2013 at 03:31:17PM +0200, Michal Privoznik wrote:
>> For some crazy backward compatibility, a console can by just an alias to
>> a serial device. This is detected in the XML formating function which
>> takes the values to format from corresponding serial device. Including
>> the device alias. This results in wrong alias being written into the XML
>> definition:
>>
>> <console type='pty' tty='/dev/pts/5'>
>> ...
>> <alias name='serial0'/>
>> </console>
>>
>> While holding the correct alias still in the memory, it doesn't matter.
>> However, it starts to matter as soon as libvirtd is restarted and the
>> (incorrect) alias is read from status file.
I don't actually see this problem at all.
Starting with a guest containing
<serial type='pty'>
<target port='0'/>
</serial>
<console type='pty'>
<target type='serial' port='0'/>
</console>
After virsh start, it contains
<serial type='pty'>
<source path='/dev/pts/7'/>
<target port='0'/>
<alias name='serial0'/>
</serial>
<console type='pty' tty='/dev/pts/7'>
<source path='/dev/pts/7'/>
<target type='serial' port='0'/>
<alias name='serial0'/>
</console>
The XML in $XDG_RUNTIME_DIR/libvirt/qemu/run/vm1.xml contains
exactly the same.
If i now kill libvirtd and restart it, the XML is still
reported correctly.
How do you reproduce the bug ?
>
> This isn't any more correct that your previous patch. For the
> dummy <console> elements, the alias *must* be copied from /
> identical to the corresponding <serial> device config.
>
>
> Daniel
>
So what are you saying is, it's the <serial> that has the wrong alias
assigned? The output should be like this then?
<serial type='pty'>
<source path='/dev/pts/5'/>
<target type='isa-serial' port='0'/>
<alias name='console0'/>
<!-- originaly was serial0 -->
</serial>
<console type='pty' tty='/dev/pts/5'>
<source path='/dev/pts/5'/>
<target type='serial' port='0'/>
<alias name='console0'/>
</console>
No, the alias name must match that of the device id= parameter in the
QEMU command line which is 'serial0' for a serial port.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
12:34 p.m.
On 28.06.2013 17:01, Daniel P. Berrange wrote:
On Fri, Jun 28, 2013 at 04:45:08PM +0200, Michal Privoznik wrote:
> On 28.06.2013 15:55, Daniel P. Berrange wrote:
>> On Fri, Jun 28, 2013 at 03:31:17PM +0200, Michal Privoznik wrote:
>>> For some crazy backward compatibility, a console can by just an alias to
>>> a serial device. This is detected in the XML formating function which
>>> takes the values to format from corresponding serial device. Including
>>> the device alias. This results in wrong alias being written into the XML
>>> definition:
>>>
>>> <console type='pty' tty='/dev/pts/5'>
>>> ...
>>> <alias name='serial0'/>
>>> </console>
>>>
>>> While holding the correct alias still in the memory, it doesn't matter.
>>> However, it starts to matter as soon as libvirtd is restarted and the
>>> (incorrect) alias is read from status file.
I don't actually see this problem at all.
Starting with a guest containing
<serial type='pty'>
<target port='0'/>
</serial>
<console type='pty'>
<target type='serial' port='0'/>
</console>
After virsh start, it contains
<serial type='pty'>
<source path='/dev/pts/7'/>
<target port='0'/>
<alias name='serial0'/>
</serial>
<console type='pty' tty='/dev/pts/7'>
<source path='/dev/pts/7'/>
<target type='serial' port='0'/>
<alias name='serial0'/>
</console>
The XML in $XDG_RUNTIME_DIR/libvirt/qemu/run/vm1.xml contains
exactly the same.
If i now kill libvirtd and restart it, the XML is still
reported correctly.
How do you reproduce the bug ?
The problem is, the internal array (vm->def->consoles) doesn't contain
just 'consoleX' but 'serialX' too after libvirtd restarts. I've
noticed
this while testing my chardev hotplug patches. If user wants to hotplug
a console, I need to iterate over vm->def->consoles to find the next
free X to generate the alias. I am using qemuDomainDeviceAliasIndex for
that. The function takes alias prefix as one of its arguments. However,
if the prefix doesn't match, an error is returned. And since the aliases
vary over libvirtd restarts I think that's the problem. Of course, I can
workaround it in my patches, but c'mon we want the libvirt source code
to be hack-less, don't we? :)
>>
>> This isn't any more correct that your previous patch. For the
>> dummy <console> elements, the alias *must* be copied from /
>> identical to the corresponding <serial> device config.
>>
>>
>> Daniel
>>
>
> So what are you saying is, it's the <serial> that has the wrong alias
> assigned? The output should be like this then?
>
> <serial type='pty'>
> <source path='/dev/pts/5'/>
> <target type='isa-serial' port='0'/>
> <alias name='console0'/>
> <!-- originaly was serial0 -->
> </serial>
>
> <console type='pty' tty='/dev/pts/5'>
> <source path='/dev/pts/5'/>
> <target type='serial' port='0'/>
> <alias name='console0'/>
> </console>
No, the alias name must match that of the device id= parameter in the
QEMU command line which is 'serial0' for a serial port.
Daniel
6:27 a.m.
On Fri, Jun 28, 2013 at 07:34:07PM +0200, Michal Privoznik wrote:
On 28.06.2013 17:01, Daniel P. Berrange wrote:
> On Fri, Jun 28, 2013 at 04:45:08PM +0200, Michal Privoznik wrote:
>> On 28.06.2013 15:55, Daniel P. Berrange wrote:
>>> On Fri, Jun 28, 2013 at 03:31:17PM +0200, Michal Privoznik wrote:
>>>> For some crazy backward compatibility, a console can by just an alias
to
>>>> a serial device. This is detected in the XML formating function which
>>>> takes the values to format from corresponding serial device. Including
>>>> the device alias. This results in wrong alias being written into the
XML
>>>> definition:
>>>>
>>>> <console type='pty' tty='/dev/pts/5'>
>>>> ...
>>>> <alias name='serial0'/>
>>>> </console>
>>>>
>>>> While holding the correct alias still in the memory, it doesn't
matter.
>>>> However, it starts to matter as soon as libvirtd is restarted and the
>>>> (incorrect) alias is read from status file.
>
> I don't actually see this problem at all.
>
> Starting with a guest containing
>
> <serial type='pty'>
> <target port='0'/>
> </serial>
> <console type='pty'>
> <target type='serial' port='0'/>
> </console>
>
> After virsh start, it contains
>
> <serial type='pty'>
> <source path='/dev/pts/7'/>
> <target port='0'/>
> <alias name='serial0'/>
> </serial>
> <console type='pty' tty='/dev/pts/7'>
> <source path='/dev/pts/7'/>
> <target type='serial' port='0'/>
> <alias name='serial0'/>
> </console>
>
>
> The XML in $XDG_RUNTIME_DIR/libvirt/qemu/run/vm1.xml contains
> exactly the same.
>
> If i now kill libvirtd and restart it, the XML is still
> reported correctly.
>
> How do you reproduce the bug ?
The problem is, the internal array (vm->def->consoles) doesn't contain
just 'consoleX' but 'serialX' too after libvirtd restarts. I've
noticed
this while testing my chardev hotplug patches. If user wants to hotplug
a console, I need to iterate over vm->def->consoles to find the next
free X to generate the alias. I am using qemuDomainDeviceAliasIndex for
that. The function takes alias prefix as one of its arguments. However,
if the prefix doesn't match, an error is returned. And since the aliases
vary over libvirtd restarts I think that's the problem. Of course, I can
workaround it in my patches, but c'mon we want the libvirt source code
to be hack-less, don't we? :)
Ok, but that's just an internal issue not visible to the user. Your
proposed patches are changing the user visible XML data for the alias
name which is wrong. So you need to find a way to address it without
causing a change in the user visible XML, which is correct as it is
now.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
6:49 a.m.
On 02.07.2013 13:27, Daniel P. Berrange wrote:
On Fri, Jun 28, 2013 at 07:34:07PM +0200, Michal Privoznik wrote:
> On 28.06.2013 17:01, Daniel P. Berrange wrote:
>> On Fri, Jun 28, 2013 at 04:45:08PM +0200, Michal Privoznik wrote:
>>> On 28.06.2013 15:55, Daniel P. Berrange wrote:
>>>> On Fri, Jun 28, 2013 at 03:31:17PM +0200, Michal Privoznik wrote:
>>>>> For some crazy backward compatibility, a console can by just an alias
to
>>>>> a serial device. This is detected in the XML formating function
which
>>>>> takes the values to format from corresponding serial device.
Including
>>>>> the device alias. This results in wrong alias being written into the
XML
>>>>> definition:
>>>>>
>>>>> <console type='pty' tty='/dev/pts/5'>
>>>>> ...
>>>>> <alias name='serial0'/>
>>>>> </console>
>>>>>
>>>>> While holding the correct alias still in the memory, it doesn't
matter.
>>>>> However, it starts to matter as soon as libvirtd is restarted and
the
>>>>> (incorrect) alias is read from status file.
>>
>> I don't actually see this problem at all.
>>
>> Starting with a guest containing
>>
>> <serial type='pty'>
>> <target port='0'/>
>> </serial>
>> <console type='pty'>
>> <target type='serial' port='0'/>
>> </console>
>>
>> After virsh start, it contains
>>
>> <serial type='pty'>
>> <source path='/dev/pts/7'/>
>> <target port='0'/>
>> <alias name='serial0'/>
>> </serial>
>> <console type='pty' tty='/dev/pts/7'>
>> <source path='/dev/pts/7'/>
>> <target type='serial' port='0'/>
>> <alias name='serial0'/>
>> </console>
>>
>>
>> The XML in $XDG_RUNTIME_DIR/libvirt/qemu/run/vm1.xml contains
>> exactly the same.
>>
>> If i now kill libvirtd and restart it, the XML is still
>> reported correctly.
>>
>> How do you reproduce the bug ?
>
> The problem is, the internal array (vm->def->consoles) doesn't contain
> just 'consoleX' but 'serialX' too after libvirtd restarts. I've
noticed
> this while testing my chardev hotplug patches. If user wants to hotplug
> a console, I need to iterate over vm->def->consoles to find the next
> free X to generate the alias. I am using qemuDomainDeviceAliasIndex for
> that. The function takes alias prefix as one of its arguments. However,
> if the prefix doesn't match, an error is returned. And since the aliases
> vary over libvirtd restarts I think that's the problem. Of course, I can
> workaround it in my patches, but c'mon we want the libvirt source code
> to be hack-less, don't we? :)
Ok, but that's just an internal issue not visible to the user. Your
proposed patches are changing the user visible XML data for the alias
name which is wrong. So you need to find a way to address it without
causing a change in the user visible XML, which is correct as it is
now.
Daniel
My first version didn't change the visible part of XML, just internal
alias representation instead.
Michal
8:25 a.m.
On Tue, Jul 02, 2013 at 01:49:35PM +0200, Michal Privoznik wrote:
On 02.07.2013 13:27, Daniel P. Berrange wrote:
> On Fri, Jun 28, 2013 at 07:34:07PM +0200, Michal Privoznik wrote:
>> On 28.06.2013 17:01, Daniel P. Berrange wrote:
>>> On Fri, Jun 28, 2013 at 04:45:08PM +0200, Michal Privoznik wrote:
>>>> On 28.06.2013 15:55, Daniel P. Berrange wrote:
>>>>> On Fri, Jun 28, 2013 at 03:31:17PM +0200, Michal Privoznik wrote:
>>>>>> For some crazy backward compatibility, a console can by just an
alias to
>>>>>> a serial device. This is detected in the XML formating function
which
>>>>>> takes the values to format from corresponding serial device.
Including
>>>>>> the device alias. This results in wrong alias being written into
the XML
>>>>>> definition:
>>>>>>
>>>>>> <console type='pty' tty='/dev/pts/5'>
>>>>>> ...
>>>>>> <alias name='serial0'/>
>>>>>> </console>
>>>>>>
>>>>>> While holding the correct alias still in the memory, it
doesn't matter.
>>>>>> However, it starts to matter as soon as libvirtd is restarted
and the
>>>>>> (incorrect) alias is read from status file.
>>>
>>> I don't actually see this problem at all.
>>>
>>> Starting with a guest containing
>>>
>>> <serial type='pty'>
>>> <target port='0'/>
>>> </serial>
>>> <console type='pty'>
>>> <target type='serial' port='0'/>
>>> </console>
>>>
>>> After virsh start, it contains
>>>
>>> <serial type='pty'>
>>> <source path='/dev/pts/7'/>
>>> <target port='0'/>
>>> <alias name='serial0'/>
>>> </serial>
>>> <console type='pty' tty='/dev/pts/7'>
>>> <source path='/dev/pts/7'/>
>>> <target type='serial' port='0'/>
>>> <alias name='serial0'/>
>>> </console>
>>>
>>>
>>> The XML in $XDG_RUNTIME_DIR/libvirt/qemu/run/vm1.xml contains
>>> exactly the same.
>>>
>>> If i now kill libvirtd and restart it, the XML is still
>>> reported correctly.
>>>
>>> How do you reproduce the bug ?
>>
>> The problem is, the internal array (vm->def->consoles) doesn't
contain
>> just 'consoleX' but 'serialX' too after libvirtd restarts.
I've noticed
>> this while testing my chardev hotplug patches. If user wants to hotplug
>> a console, I need to iterate over vm->def->consoles to find the next
>> free X to generate the alias. I am using qemuDomainDeviceAliasIndex for
>> that. The function takes alias prefix as one of its arguments. However,
>> if the prefix doesn't match, an error is returned. And since the aliases
>> vary over libvirtd restarts I think that's the problem. Of course, I can
>> workaround it in my patches, but c'mon we want the libvirt source code
>> to be hack-less, don't we? :)
>
> Ok, but that's just an internal issue not visible to the user. Your
> proposed patches are changing the user visible XML data for the alias
> name which is wrong. So you need to find a way to address it without
> causing a change in the user visible XML, which is correct as it is
> now.
>
> Daniel
>
My first version didn't change the visible part of XML, just internal
alias representation instead.
Hmm, ok the commit message was a bit misleading - it looked like you
were saying the XML was wrong & the patch was changing it.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4195
days inactive
4199
days old
7 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Michal Privoznik