[Spice-devel,v4] qemu: Use heads parameter for QXL driver

Submitted by Frediano Ziglio on July 17, 2015, 8:29 a.m.

Details

Message ID 1437121784-10136-1-git-send-email-fziglio@redhat.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Frediano Ziglio July 17, 2015, 8:29 a.m.
Allows to specify maximum number of head to QXL driver.

Actually can be a compatiblity problem as heads in the XML configuration
was set by default to '1'.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 src/qemu/qemu_capabilities.c                       |  2 ++
 src/qemu/qemu_capabilities.h                       |  1 +
 src/qemu/qemu_command.c                            |  5 ++++
 .../qemuxml2argv-video-qxl-device-max-outputs.args |  7 ++++++
 .../qemuxml2argv-video-qxl-device-max-outputs.xml  | 29 ++++++++++++++++++++++
 tests/qemuxml2argvtest.c                           |  3 +++
 6 files changed, 47 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.xml

Changes from v4:
- rebased on new master;
- add test case to qemuxml2argvtest.

Patch hide | download patch | download mbox

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d8cb32d..0769316 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -288,6 +288,7 @@  VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
 
               "vhost-user-multiqueue", /* 190 */
               "migration-event",
+              "qxl-vga.max_outputs",
     );
 
 
@@ -1651,6 +1652,7 @@  static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxl[] = {
 
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxlVga[] = {
     { "vgamem_mb", QEMU_CAPS_QXL_VGA_VGAMEM },
+    { "max_outputs", QEMU_CAPS_QXL_VGA_MAX_OUTPUTS },
 };
 
 struct virQEMUCapsObjectTypeProps {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index f77bd06..88116af 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -231,6 +231,7 @@  typedef enum {
     QEMU_CAPS_CPU_AARCH64_OFF    = 189, /* -cpu ...,aarch64=off */
     QEMU_CAPS_VHOSTUSER_MULTIQUEUE = 190, /* vhost-user with -netdev queues= */
     QEMU_CAPS_MIGRATION_EVENT    = 191, /* MIGRATION event */
+    QEMU_CAPS_QXL_VGA_MAX_OUTPUTS = 192, /* qxl-vga.max_outputs */
 
     QEMU_CAPS_LAST,                   /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 42906a8..34a0574 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5684,6 +5684,11 @@  qemuBuildDeviceVideoStr(virDomainDefPtr def,
             /* QEMU accepts mebibytes for vgamem_mb. */
             virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vgamem / 1024);
         }
+
+        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QXL_VGA_MAX_OUTPUTS) &&
+            video->heads > 0) {
+            virBufferAsprintf(&buf, ",max_outputs=%u", video->heads);
+        }
     } else if (video->vram &&
         ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
           virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM)) ||
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.args b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.args
new file mode 100644
index 0000000..bb477a9
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.args
@@ -0,0 +1,7 @@ 
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
+/usr/bin/qemu -S -M pc -m 1024 -smp 1 -nographic -nodefaults \
+-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \
+-hda /var/lib/libvirt/images/QEMUGuest1 \
+-device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vgamem_mb=16\
+,max_outputs=3,bus=pci.0,addr=0x2 -device virtio-balloon-pci,id=balloon0\
+,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.xml b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.xml
new file mode 100644
index 0000000..9426efc
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.xml
@@ -0,0 +1,29 @@ 
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>1048576</memory>
+  <currentMemory unit='KiB'>1048576</currentMemory>
+  <vcpu>1</vcpu>
+  <os>
+    <type arch='i686' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu</emulator>
+    <disk type='file' device='disk'>
+      <driver name='qemu' type='qcow2' cache='none'/>
+      <source file='/var/lib/libvirt/images/QEMUGuest1'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <controller type='ide' index='0'/>
+    <video>
+      <model type='qxl' heads='3'/>
+    </video>
+    <memballoon model='virtio'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index f9b30d9..4cd6892 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1441,6 +1441,9 @@  mymain(void)
     DO_TEST("video-qxl-device-vgamem", QEMU_CAPS_DEVICE,
             QEMU_CAPS_DEVICE_QXL_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
             QEMU_CAPS_QXL_VGA_VGAMEM);
+    DO_TEST("video-qxl-device-max-outputs", QEMU_CAPS_DEVICE,
+            QEMU_CAPS_DEVICE_QXL_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
+            QEMU_CAPS_QXL_VGA_VGAMEM, QEMU_CAPS_QXL_VGA_MAX_OUTPUTS);
     DO_TEST_FAILURE("video-qxl-sec-nodevice", QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL);
     DO_TEST("video-qxl-sec-device", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_QXL_VGA,
             QEMU_CAPS_DEVICE_QXL, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);

Comments

On Fri, Jul 17, 2015 at 09:29:44 +0100, Frediano Ziglio wrote:
> Allows to specify maximum number of head to QXL driver.
> 
> Actually can be a compatiblity problem as heads in the XML configuration
> was set by default to '1'.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  src/qemu/qemu_capabilities.c                       |  2 ++
>  src/qemu/qemu_capabilities.h                       |  1 +
>  src/qemu/qemu_command.c                            |  5 ++++
>  .../qemuxml2argv-video-qxl-device-max-outputs.args |  7 ++++++
>  .../qemuxml2argv-video-qxl-device-max-outputs.xml  | 29 ++++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           |  3 +++
>  6 files changed, 47 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.xml
> 
> Changes from v4:
> - rebased on new master;
> - add test case to qemuxml2argvtest.

Note review: This was added in qemu commit
http://git.qemu.org/?p=qemu.git;a=commitdiff;h=567161fdd47aeb6987e700702f6bbfef04ae0236

The commit hints that you need at least spice 0.12.6. I'll update spice
and test this later.
On Fri, Jul 17, 2015 at 12:11:55PM +0200, Peter Krempa wrote:
>On Fri, Jul 17, 2015 at 09:29:44 +0100, Frediano Ziglio wrote:
>> Allows to specify maximum number of head to QXL driver.
>>
>> Actually can be a compatiblity problem as heads in the XML configuration
>> was set by default to '1'.
>>
>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>> ---
>>  src/qemu/qemu_capabilities.c                       |  2 ++
>>  src/qemu/qemu_capabilities.h                       |  1 +
>>  src/qemu/qemu_command.c                            |  5 ++++
>>  .../qemuxml2argv-video-qxl-device-max-outputs.args |  7 ++++++
>>  .../qemuxml2argv-video-qxl-device-max-outputs.xml  | 29 ++++++++++++++++++++++
>>  tests/qemuxml2argvtest.c                           |  3 +++
>>  6 files changed, 47 insertions(+)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.xml
>>
>> Changes from v4:
>> - rebased on new master;
>> - add test case to qemuxml2argvtest.
>
>Note review: This was added in qemu commit
>http://git.qemu.org/?p=qemu.git;a=commitdiff;h=567161fdd47aeb6987e700702f6bbfef04ae0236
>
>The commit hints that you need at least spice 0.12.6. I'll update spice
>and test this later.

I failed to update qemu without updating spice as well, so I'd say
it is safe to assume that if qemu supports it, spice will too.  And we
don't check all the lib versions for qemu runtime, that'd be a waste
of time, so this is quite alright, I think.

I updated everything spice-related and I still can't properly build
qemu particularly because of the patch you mentioned.  And I update
everything from git.  Then I looked there and I see no such function,
no wonder the build fails with undefined reference to
spice_qxl_set_monitors_config_limit.  Although that would mean that it
found a header file with the declaration...  Anyway, the patch looks
fine and even though I would normally want a split into at least two
parts, this is small enough that I'm fine with it being one patch
only.

I'll see next week (or maybe over the weekend) how the build and
everything goes unless someone beats me to it.

Martin
> 
> On Fri, Jul 17, 2015 at 12:11:55PM +0200, Peter Krempa wrote:
> >On Fri, Jul 17, 2015 at 09:29:44 +0100, Frediano Ziglio wrote:
> >> Allows to specify maximum number of head to QXL driver.
> >>
> >> Actually can be a compatiblity problem as heads in the XML configuration
> >> was set by default to '1'.
> >>
> >> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> >> ---
> >>  src/qemu/qemu_capabilities.c                       |  2 ++
> >>  src/qemu/qemu_capabilities.h                       |  1 +
> >>  src/qemu/qemu_command.c                            |  5 ++++
> >>  .../qemuxml2argv-video-qxl-device-max-outputs.args |  7 ++++++
> >>  .../qemuxml2argv-video-qxl-device-max-outputs.xml  | 29
> >>  ++++++++++++++++++++++
> >>  tests/qemuxml2argvtest.c                           |  3 +++
> >>  6 files changed, 47 insertions(+)
> >>  create mode 100644
> >>  tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.args
> >>  create mode 100644
> >>  tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.xml
> >>
> >> Changes from v4:
> >> - rebased on new master;
> >> - add test case to qemuxml2argvtest.
> >
> >Note review: This was added in qemu commit
> >http://git.qemu.org/?p=qemu.git;a=commitdiff;h=567161fdd47aeb6987e700702f6bbfef04ae0236
> >
> >The commit hints that you need at least spice 0.12.6. I'll update spice
> >and test this later.
> 
> I failed to update qemu without updating spice as well, so I'd say
> it is safe to assume that if qemu supports it, spice will too.  And we
> don't check all the lib versions for qemu runtime, that'd be a waste
> of time, so this is quite alright, I think.
> 

Qemu should detect spice-server version and enable that feature.
So Qemu should build with either this feature or not. The only problem would be a development 0.12.6 version without my patch but if you used an updated git should not be the point.
I updated everything and tested Qemu limited the outputs.

> I updated everything spice-related and I still can't properly build
> qemu particularly because of the patch you mentioned.  And I update
> everything from git.  Then I looked there and I see no such function,
> no wonder the build fails with undefined reference to
> spice_qxl_set_monitors_config_limit.  Although that would mean that it
> found a header file with the declaration...  Anyway, the patch looks
> fine and even though I would normally want a split into at least two
> parts, this is small enough that I'm fine with it being one patch
> only.
> 
> I'll see next week (or maybe over the weekend) how the build and
> everything goes unless someone beats me to it.
> 
> Martin
> 

Frediano
On Fri, Jul 17, 2015 at 03:42:36PM +0200, Martin Kletzander wrote:
> On Fri, Jul 17, 2015 at 12:11:55PM +0200, Peter Krempa wrote:
> >On Fri, Jul 17, 2015 at 09:29:44 +0100, Frediano Ziglio wrote:
> >>Allows to specify maximum number of head to QXL driver.
> >>
> >>Actually can be a compatiblity problem as heads in the XML configuration
> >>was set by default to '1'.
> >>
> >>Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> >>---
> >> src/qemu/qemu_capabilities.c                       |  2 ++
> >> src/qemu/qemu_capabilities.h                       |  1 +
> >> src/qemu/qemu_command.c                            |  5 ++++
> >> .../qemuxml2argv-video-qxl-device-max-outputs.args |  7 ++++++
> >> .../qemuxml2argv-video-qxl-device-max-outputs.xml  | 29 ++++++++++++++++++++++
> >> tests/qemuxml2argvtest.c                           |  3 +++
> >> 6 files changed, 47 insertions(+)
> >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.args
> >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.xml
> >>
> >>Changes from v4:
> >>- rebased on new master;
> >>- add test case to qemuxml2argvtest.
> >
> >Note review: This was added in qemu commit
> >http://git.qemu.org/?p=qemu.git;a=commitdiff;h=567161fdd47aeb6987e700702f6bbfef04ae0236
> >
> >The commit hints that you need at least spice 0.12.6. I'll update spice
> >and test this later.
> 
> I failed to update qemu without updating spice as well, so I'd say
> it is safe to assume that if qemu supports it, spice will too.  And we
> don't check all the lib versions for qemu runtime, that'd be a waste
> of time, so this is quite alright, I think.
> 
> I updated everything spice-related and I still can't properly build
> qemu particularly because of the patch you mentioned.  And I update
> everything from git.  Then I looked there and I see no such function,
> no wonder the build fails with undefined reference to
> spice_qxl_set_monitors_config_limit.  Although that would mean that it

qemu tries to call spice_qxl_set_monitors_config_limit but the entry
point ended up being called spice_qxl_set_max_monitors in spice-server
:-/

Christophe
On Fri, Jul 17, 2015 at 06:02:25PM +0200, Christophe Fergeau wrote:
>On Fri, Jul 17, 2015 at 03:42:36PM +0200, Martin Kletzander wrote:
>> On Fri, Jul 17, 2015 at 12:11:55PM +0200, Peter Krempa wrote:
>> >On Fri, Jul 17, 2015 at 09:29:44 +0100, Frediano Ziglio wrote:
>> >>Allows to specify maximum number of head to QXL driver.
>> >>
>> >>Actually can be a compatiblity problem as heads in the XML configuration
>> >>was set by default to '1'.
>> >>
>> >>Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>> >>---
>> >> src/qemu/qemu_capabilities.c                       |  2 ++
>> >> src/qemu/qemu_capabilities.h                       |  1 +
>> >> src/qemu/qemu_command.c                            |  5 ++++
>> >> .../qemuxml2argv-video-qxl-device-max-outputs.args |  7 ++++++
>> >> .../qemuxml2argv-video-qxl-device-max-outputs.xml  | 29 ++++++++++++++++++++++
>> >> tests/qemuxml2argvtest.c                           |  3 +++
>> >> 6 files changed, 47 insertions(+)
>> >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.args
>> >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.xml
>> >>
>> >>Changes from v4:
>> >>- rebased on new master;
>> >>- add test case to qemuxml2argvtest.
>> >
>> >Note review: This was added in qemu commit
>> >http://git.qemu.org/?p=qemu.git;a=commitdiff;h=567161fdd47aeb6987e700702f6bbfef04ae0236
>> >
>> >The commit hints that you need at least spice 0.12.6. I'll update spice
>> >and test this later.
>>
>> I failed to update qemu without updating spice as well, so I'd say
>> it is safe to assume that if qemu supports it, spice will too.  And we
>> don't check all the lib versions for qemu runtime, that'd be a waste
>> of time, so this is quite alright, I think.
>>
>> I updated everything spice-related and I still can't properly build
>> qemu particularly because of the patch you mentioned.  And I update
>> everything from git.  Then I looked there and I see no such function,
>> no wonder the build fails with undefined reference to
>> spice_qxl_set_monitors_config_limit.  Although that would mean that it
>
>qemu tries to call spice_qxl_set_monitors_config_limit but the entry
>point ended up being called spice_qxl_set_max_monitors in spice-server
>:-/
>

I spend all morning fixing this to be installed properly in the
system.  Anyway, I finally managed to make this work and found out the
guest I used for it is not ready to have multiple monitors.  Anyway,
looking at everything else this definitely works, so ACK, I'll push it
in a while.

In the meantime, is the only thing this does limiting the maximum?  Is
it there just to save some memory or why?  Because otherwise I can't
see the use-case in that.  I'm not saying there isn't one, just that I
can't find it.  And I even looked under the fridge :)

Have a nice day,
Martin
Hi,

On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
> In the meantime, is the only thing this does limiting the maximum?  Is
> it there just to save some memory or why?  Because otherwise I can't
> see the use-case in that.  I'm not saying there isn't one, just that I
> can't find it.  And I even looked under the fridge :)

There are 2 main uses for this feature:
- first, it's indeed to help in setting up guest memory size, you need a
  bit more than 16MB of VRAM per full-HD display that you want your
  guest to support, and the failure modes when the guest tries to go
  over that limit are not explicit at all. Thus it can be helpful to
  enforce the limitation to just 1 monitor if you don't want to assign a
  lot of VRAM to your guest
- the second one is more cosmetic, management innterfaces (oVirt) let
  you select the number of screens you want in the VM, but at the moment
  it's not used at all on the SPICE client side, you always get a list
  of 4 displays which you can enable in one of the client menus. After
  the work Frediano has done, you only see 1 entry in that client menu
  if you said in the VM XML that you only want 1 head.

Hope that helps,

Christophe
On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
> I spend all morning fixing this to be installed properly in the
> system.  Anyway, I finally managed to make this work and found out the
> guest I used for it is not ready to have multiple monitors.  Anyway,
> looking at everything else this definitely works, so ACK, I'll push it
> in a while.

I'm still a bit worried that all existing guests will have heads='1' in
their XML as libvirt is adding that by default, but I don't really see a
way around it :-/ We should make sure libvirt stops adding it from now
on though ;)

Christophe
On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
>On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
>> I spend all morning fixing this to be installed properly in the
>> system.  Anyway, I finally managed to make this work and found out the
>> guest I used for it is not ready to have multiple monitors.  Anyway,
>> looking at everything else this definitely works, so ACK, I'll push it
>> in a while.
>
>I'm still a bit worried that all existing guests will have heads='1' in
>their XML as libvirt is adding that by default, but I don't really see a
>way around it :-/ We should make sure libvirt stops adding it from now
>on though ;)
>

Well, how would you then limit the outputs to 1?  Having said that, I
have no idea why we started adding heads="1" automatically and playing
with different changes, we have another bug in the parsing/formatting
code.  You'll also won't be able to migrate from older libvirt with
heads='1' to newer ones.  I haven't realized that.  I'm thinking about
reverting the change in libvirt or even using heads > 1.  Although
that won't migrate either.  So the only other thing that we can do is
to introduce new parameter, like maxHeads.  The sound you just heard
is me slapping my face because again there will multiple parameters
meaning similar things...

>Christophe
> 
> On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
> >On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
> >> I spend all morning fixing this to be installed properly in the
> >> system.  Anyway, I finally managed to make this work and found out the
> >> guest I used for it is not ready to have multiple monitors.  Anyway,
> >> looking at everything else this definitely works, so ACK, I'll push it
> >> in a while.
> >
> >I'm still a bit worried that all existing guests will have heads='1' in
> >their XML as libvirt is adding that by default, but I don't really see a
> >way around it :-/ We should make sure libvirt stops adding it from now
> >on though ;)
> >
> 
> Well, how would you then limit the outputs to 1?  Having said that, I
> have no idea why we started adding heads="1" automatically and playing
> with different changes, we have another bug in the parsing/formatting
> code.  You'll also won't be able to migrate from older libvirt with
> heads='1' to newer ones.  I haven't realized that.  I'm thinking about
> reverting the change in libvirt or even using heads > 1.  Although
> that won't migrate either.  So the only other thing that we can do is
> to introduce new parameter, like maxHeads.  The sound you just heard
> is me slapping my face because again there will multiple parameters
> meaning similar things...
> 

heads specify the number of heads the emulated virtual card has. I think the problem was specify the number for QXL which never honored this setting.
I don't think you can't migrate, just after migration the machine won't allow more than 1 head.
Unfortunately the XML file does not have any field to specify the libvirt version was meant for. It would be useful to add so if you see the version it means that heads==1 means 1 if not it means is not defined.
I'm against a new parameter which specify the same meaning of another parameter. I would instead something like validHeads or headsSet with a false as default.

> >Christophe
> 

Frediano
On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
> On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
> >On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
> >>I spend all morning fixing this to be installed properly in the
> >>system.  Anyway, I finally managed to make this work and found out the
> >>guest I used for it is not ready to have multiple monitors.  Anyway,
> >>looking at everything else this definitely works, so ACK, I'll push it
> >>in a while.
> >
> >I'm still a bit worried that all existing guests will have heads='1' in
> >their XML as libvirt is adding that by default, but I don't really see a
> >way around it :-/ We should make sure libvirt stops adding it from now
> >on though ;)
> >
> 
> Well, how would you then limit the outputs to 1?  Having said that, I
> have no idea why we started adding heads="1" automatically and playing
> with different changes, we have another bug in the parsing/formatting
> code.  You'll also won't be able to migrate from older libvirt with
> heads='1' to newer ones.  I haven't realized that.  I'm thinking about
> reverting the change in libvirt or even using heads > 1.  Although
> that won't migrate either.  So the only other thing that we can do is
> to introduce new parameter, like maxHeads.  The sound you just heard
> is me slapping my face because again there will multiple parameters
> meaning similar things...

Introducing a new parameter is not really viable IMHO, because as you
say it'll leave us with two things meaning the same, and will not be
compatible with existing apps that know about the current parameter.

I think we need to figure out a way to drop the 'heads=1' from any
existing configs in /etc/libvirt/qemu when we start up with the new
libvirtd for the first time.

To make this work we probably need to write a magic attribute or
comment into the XML configs to record which version of libvirt
saved it to disk. That way we know whether this is the first time
loading the config with the new libvirt. It will help us with
similar situations in the future. In this case, we'd just look
to see if the libvirt version number was missing from the XML,
and if so, then drop heads=1. If a version number is present, then
we're new enough, so we can keep it.  We'd also ned to pass this
version number in the XML when migrating, or possibly via the
migration cookie.

Regards,
Daniel
On Tue, Jul 21, 2015 at 08:41:33AM -0400, Frediano Ziglio wrote:
>>
>> On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
>> >On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
>> >> I spend all morning fixing this to be installed properly in the
>> >> system.  Anyway, I finally managed to make this work and found out the
>> >> guest I used for it is not ready to have multiple monitors.  Anyway,
>> >> looking at everything else this definitely works, so ACK, I'll push it
>> >> in a while.
>> >
>> >I'm still a bit worried that all existing guests will have heads='1' in
>> >their XML as libvirt is adding that by default, but I don't really see a
>> >way around it :-/ We should make sure libvirt stops adding it from now
>> >on though ;)
>> >
>>
>> Well, how would you then limit the outputs to 1?  Having said that, I
>> have no idea why we started adding heads="1" automatically and playing
>> with different changes, we have another bug in the parsing/formatting
>> code.  You'll also won't be able to migrate from older libvirt with
>> heads='1' to newer ones.  I haven't realized that.  I'm thinking about
>> reverting the change in libvirt or even using heads > 1.  Although
>> that won't migrate either.  So the only other thing that we can do is
>> to introduce new parameter, like maxHeads.  The sound you just heard
>> is me slapping my face because again there will multiple parameters
>> meaning similar things...
>>
>
>heads specify the number of heads the emulated virtual card has. I think the problem was specify the number for QXL which never honored this setting.
>I don't think you can't migrate, just after migration the machine won't allow more than 1 head.
>Unfortunately the XML file does not have any field to specify the libvirt version was meant for. It would be useful to add so if you see the version it means that heads==1 means 1 if not it means is not defined.
>I'm against a new parameter which specify the same meaning of another parameter. I would instead something like validHeads or headsSet with a false as default.
>

[Would you mind convincing your mail client to wrap long lines?]

Well, libvirt is, unfortunately, by definition, backward compatible.
So we guarantee, that you don't have to change this stuff and you can
migrate to newer ones.  The problem here with the migration is that
the guests will be binary incompatible, the amount of memory you need
to transfer from source will not fit the destination.

Until we reach a decision, I would revert the patch.  We need to
decide how to handle this.

About the new name.  I take it as 'heads' defines how many monitors
there are and 'maxHeads' would define the maximum of them to have.
The difference would be there of course only for qxl.  I don't think
cirrus, for example, can dynamically change number of monitors
connected, can it?  We had similar problem with the ram/vram/vgaram
attributes and we ended up with all of them and bunch of lines dealing
with just the update and migration compatibility.

Having headsSet is pointless.  How would you decide whether heads is
actually 1 or not if you were to parse an XML with heads='1'?
Dropping heads='1' is not an option as well since that would make
other video models (that support heads=) change their settings.  And
nobody could set heads='1' any more without also setting another
parameter.  The problem is we screwed up when we defaulted heads to a
number even though there are devices that do not handle that
properly.  That's the past, we can't do much about that due to
back-compat (Aargh!), but that's how it is, unfortunately).

That are my thoughts on why maxHeads should be a new parameter that
would set max_outputs for the qxl device.
On Tue, Jul 21, 2015 at 01:50:22PM +0100, Daniel P. Berrange wrote:
>On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
>> On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
>> >On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
>> >>I spend all morning fixing this to be installed properly in the
>> >>system.  Anyway, I finally managed to make this work and found out the
>> >>guest I used for it is not ready to have multiple monitors.  Anyway,
>> >>looking at everything else this definitely works, so ACK, I'll push it
>> >>in a while.
>> >
>> >I'm still a bit worried that all existing guests will have heads='1' in
>> >their XML as libvirt is adding that by default, but I don't really see a
>> >way around it :-/ We should make sure libvirt stops adding it from now
>> >on though ;)
>> >
>>
>> Well, how would you then limit the outputs to 1?  Having said that, I
>> have no idea why we started adding heads="1" automatically and playing
>> with different changes, we have another bug in the parsing/formatting
>> code.  You'll also won't be able to migrate from older libvirt with
>> heads='1' to newer ones.  I haven't realized that.  I'm thinking about
>> reverting the change in libvirt or even using heads > 1.  Although
>> that won't migrate either.  So the only other thing that we can do is
>> to introduce new parameter, like maxHeads.  The sound you just heard
>> is me slapping my face because again there will multiple parameters
>> meaning similar things...
>
>Introducing a new parameter is not really viable IMHO, because as you
>say it'll leave us with two things meaning the same, and will not be
>compatible with existing apps that know about the current parameter.
>
>I think we need to figure out a way to drop the 'heads=1' from any
>existing configs in /etc/libvirt/qemu when we start up with the new
>libvirtd for the first time.
>

I'm already working on an RFC that would differentiate between loading
XMLs on daemon start-up and defining them.  The purpose of that is so
that we can make incompatible XML changes without losing any domains,
but that's not the point here.  If we were to drop heads='1' anywhere,
this is the place.  The ABI change would be minimal for the guest.

>To make this work we probably need to write a magic attribute or
>comment into the XML configs to record which version of libvirt
>saved it to disk. That way we know whether this is the first time
>loading the config with the new libvirt. It will help us with
>similar situations in the future. In this case, we'd just look
>to see if the libvirt version number was missing from the XML,
>and if so, then drop heads=1. If a version number is present, then
>we're new enough, so we can keep it.  We'd also ned to pass this
>version number in the XML when migrating, or possibly via the
>migration cookie.
>

And you're suggesting to differentiate this by a comment?  That's
really, *really* hacky.  Anyway, do you agree on temporarily reverting
the commit so we don't release another version with it?  The changes
will also be clearer when they will not modify what this commit
changed.

>Regards,
>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 :|
On Tue, Jul 21, 2015 at 03:35:50PM +0200, Martin Kletzander wrote:
> On Tue, Jul 21, 2015 at 01:50:22PM +0100, Daniel P. Berrange wrote:
> >On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
> >>On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
> >>>On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
> >>>>I spend all morning fixing this to be installed properly in the
> >>>>system.  Anyway, I finally managed to make this work and found out the
> >>>>guest I used for it is not ready to have multiple monitors.  Anyway,
> >>>>looking at everything else this definitely works, so ACK, I'll push it
> >>>>in a while.
> >>>
> >>>I'm still a bit worried that all existing guests will have heads='1' in
> >>>their XML as libvirt is adding that by default, but I don't really see a
> >>>way around it :-/ We should make sure libvirt stops adding it from now
> >>>on though ;)
> >>>
> >>
> >>Well, how would you then limit the outputs to 1?  Having said that, I
> >>have no idea why we started adding heads="1" automatically and playing
> >>with different changes, we have another bug in the parsing/formatting
> >>code.  You'll also won't be able to migrate from older libvirt with
> >>heads='1' to newer ones.  I haven't realized that.  I'm thinking about
> >>reverting the change in libvirt or even using heads > 1.  Although
> >>that won't migrate either.  So the only other thing that we can do is
> >>to introduce new parameter, like maxHeads.  The sound you just heard
> >>is me slapping my face because again there will multiple parameters
> >>meaning similar things...
> >
> >Introducing a new parameter is not really viable IMHO, because as you
> >say it'll leave us with two things meaning the same, and will not be
> >compatible with existing apps that know about the current parameter.
> >
> >I think we need to figure out a way to drop the 'heads=1' from any
> >existing configs in /etc/libvirt/qemu when we start up with the new
> >libvirtd for the first time.
> >
> 
> I'm already working on an RFC that would differentiate between loading
> XMLs on daemon start-up and defining them.  The purpose of that is so
> that we can make incompatible XML changes without losing any domains,
> but that's not the point here.  If we were to drop heads='1' anywhere,
> this is the place.  The ABI change would be minimal for the guest.

It isn't sufficient to just differentiate loading on startup
vs defining them IIUC. We need to differentiate loading on
startup from XML previously written by a libvirtd < X.Y.Z
which is why I think we need to have a version number recorded
in the XML ultimately.

> >To make this work we probably need to write a magic attribute or
> >comment into the XML configs to record which version of libvirt
> >saved it to disk. That way we know whether this is the first time
> >loading the config with the new libvirt. It will help us with
> >similar situations in the future. In this case, we'd just look
> >to see if the libvirt version number was missing from the XML,
> >and if so, then drop heads=1. If a version number is present, then
> >we're new enough, so we can keep it.  We'd also ned to pass this
> >version number in the XML when migrating, or possibly via the
> >migration cookie.
> >
> 
> And you're suggesting to differentiate this by a comment?  That's
> really, *really* hacky.  Anyway, do you agree on temporarily reverting
> the commit so we don't release another version with it?  The changes
> will also be clearer when they will not modify what this commit
> changed.

Actually we'd need to have a formal attribute to deal with the migration
case, so slightly less hacky.

Yeah, if we can't immediately fix it, we should revert it until we have
a decent fix without regression. Likewise probably fix it on stable
branch too.

Regards,
Daniel
On 07/21/2015 09:41 AM, Daniel P. Berrange wrote:
> On Tue, Jul 21, 2015 at 03:35:50PM +0200, Martin Kletzander wrote:
>> On Tue, Jul 21, 2015 at 01:50:22PM +0100, Daniel P. Berrange wrote:
>>> On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
>>>> On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
>>>>> On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
>>>>>> I spend all morning fixing this to be installed properly in the
>>>>>> system.  Anyway, I finally managed to make this work and found out the
>>>>>> guest I used for it is not ready to have multiple monitors.  Anyway,
>>>>>> looking at everything else this definitely works, so ACK, I'll push it
>>>>>> in a while.
>>>>> I'm still a bit worried that all existing guests will have heads='1' in
>>>>> their XML as libvirt is adding that by default, but I don't really see a
>>>>> way around it :-/ We should make sure libvirt stops adding it from now
>>>>> on though ;)
>>>>>
>>>> Well, how would you then limit the outputs to 1?  Having said that, I
>>>> have no idea why we started adding heads="1" automatically and playing
>>>> with different changes, we have another bug in the parsing/formatting
>>>> code.  You'll also won't be able to migrate from older libvirt with
>>>> heads='1' to newer ones.  I haven't realized that.  I'm thinking about
>>>> reverting the change in libvirt or even using heads > 1.  Although
>>>> that won't migrate either.  So the only other thing that we can do is
>>>> to introduce new parameter, like maxHeads.  The sound you just heard
>>>> is me slapping my face because again there will multiple parameters
>>>> meaning similar things...
>>> Introducing a new parameter is not really viable IMHO, because as you
>>> say it'll leave us with two things meaning the same, and will not be
>>> compatible with existing apps that know about the current parameter.
>>>
>>> I think we need to figure out a way to drop the 'heads=1' from any
>>> existing configs in /etc/libvirt/qemu when we start up with the new
>>> libvirtd for the first time.
>>>
>> I'm already working on an RFC that would differentiate between loading
>> XMLs on daemon start-up and defining them.  The purpose of that is so
>> that we can make incompatible XML changes without losing any domains,
>> but that's not the point here.  If we were to drop heads='1' anywhere,
>> this is the place.  The ABI change would be minimal for the guest.
> It isn't sufficient to just differentiate loading on startup
> vs defining them IIUC. We need to differentiate loading on
> startup from XML previously written by a libvirtd < X.Y.Z
> which is why I think we need to have a version number recorded
> in the XML ultimately.

But a version number isn't sufficient to describe exactly  what the
previous libvirt was capable of. Many times new features (externally
visible only in the XML) are backported to earlier versions of libvirt
downstream (e.g. libvirt-0.10.2 that is used in RHEL6.x and CentOS 6.x),
so this still doesn't buy us perfection. Downstream maintainers could
make it better by paying very close attention to any use of this version
number when backporting new stuff, but there would still be problems if
someone decided to build and install an upstream libvirt on a system
that previously had some hybrid downstream build.

(Not saying we shouldn't do it, just that it's no perfect :-)
On Tue, Jul 21, 2015 at 11:34:08AM -0400, Laine Stump wrote:
> On 07/21/2015 09:41 AM, Daniel P. Berrange wrote:
> > On Tue, Jul 21, 2015 at 03:35:50PM +0200, Martin Kletzander wrote:
> >> On Tue, Jul 21, 2015 at 01:50:22PM +0100, Daniel P. Berrange wrote:
> >>> On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
> >>>> On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
> >>>>> On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
> >>>>>> I spend all morning fixing this to be installed properly in the
> >>>>>> system.  Anyway, I finally managed to make this work and found out the
> >>>>>> guest I used for it is not ready to have multiple monitors.  Anyway,
> >>>>>> looking at everything else this definitely works, so ACK, I'll push it
> >>>>>> in a while.
> >>>>> I'm still a bit worried that all existing guests will have heads='1' in
> >>>>> their XML as libvirt is adding that by default, but I don't really see a
> >>>>> way around it :-/ We should make sure libvirt stops adding it from now
> >>>>> on though ;)
> >>>>>
> >>>> Well, how would you then limit the outputs to 1?  Having said that, I
> >>>> have no idea why we started adding heads="1" automatically and playing
> >>>> with different changes, we have another bug in the parsing/formatting
> >>>> code.  You'll also won't be able to migrate from older libvirt with
> >>>> heads='1' to newer ones.  I haven't realized that.  I'm thinking about
> >>>> reverting the change in libvirt or even using heads > 1.  Although
> >>>> that won't migrate either.  So the only other thing that we can do is
> >>>> to introduce new parameter, like maxHeads.  The sound you just heard
> >>>> is me slapping my face because again there will multiple parameters
> >>>> meaning similar things...
> >>> Introducing a new parameter is not really viable IMHO, because as you
> >>> say it'll leave us with two things meaning the same, and will not be
> >>> compatible with existing apps that know about the current parameter.
> >>>
> >>> I think we need to figure out a way to drop the 'heads=1' from any
> >>> existing configs in /etc/libvirt/qemu when we start up with the new
> >>> libvirtd for the first time.
> >>>
> >> I'm already working on an RFC that would differentiate between loading
> >> XMLs on daemon start-up and defining them.  The purpose of that is so
> >> that we can make incompatible XML changes without losing any domains,
> >> but that's not the point here.  If we were to drop heads='1' anywhere,
> >> this is the place.  The ABI change would be minimal for the guest.
> > It isn't sufficient to just differentiate loading on startup
> > vs defining them IIUC. We need to differentiate loading on
> > startup from XML previously written by a libvirtd < X.Y.Z
> > which is why I think we need to have a version number recorded
> > in the XML ultimately.
> 
> But a version number isn't sufficient to describe exactly  what the
> previous libvirt was capable of. Many times new features (externally
> visible only in the XML) are backported to earlier versions of libvirt
> downstream (e.g. libvirt-0.10.2 that is used in RHEL6.x and CentOS 6.x),
> so this still doesn't buy us perfection. Downstream maintainers could
> make it better by paying very close attention to any use of this version
> number when backporting new stuff, but there would still be problems if
> someone decided to build and install an upstream libvirt on a system
> that previously had some hybrid downstream build.
> 
> (Not saying we shouldn't do it, just that it's no perfect :-)

Yep, understood. I'm thinking purely in terms of upstream where we do
not backport features to stable branches. Distros which get into the
feature backport game have to deal with that pain themselves.

Regards,
Daniel
On Tue, Jul 21, 2015 at 04:36:45PM +0100, Daniel P. Berrange wrote:
>On Tue, Jul 21, 2015 at 11:34:08AM -0400, Laine Stump wrote:
>> On 07/21/2015 09:41 AM, Daniel P. Berrange wrote:
>> > On Tue, Jul 21, 2015 at 03:35:50PM +0200, Martin Kletzander wrote:
>> >> On Tue, Jul 21, 2015 at 01:50:22PM +0100, Daniel P. Berrange wrote:
>> >>> On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
>> >>>> On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
>> >>>>> On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
>> >>>>>> I spend all morning fixing this to be installed properly in the
>> >>>>>> system.  Anyway, I finally managed to make this work and found out the
>> >>>>>> guest I used for it is not ready to have multiple monitors.  Anyway,
>> >>>>>> looking at everything else this definitely works, so ACK, I'll push it
>> >>>>>> in a while.
>> >>>>> I'm still a bit worried that all existing guests will have heads='1' in
>> >>>>> their XML as libvirt is adding that by default, but I don't really see a
>> >>>>> way around it :-/ We should make sure libvirt stops adding it from now
>> >>>>> on though ;)
>> >>>>>
>> >>>> Well, how would you then limit the outputs to 1?  Having said that, I
>> >>>> have no idea why we started adding heads="1" automatically and playing
>> >>>> with different changes, we have another bug in the parsing/formatting
>> >>>> code.  You'll also won't be able to migrate from older libvirt with
>> >>>> heads='1' to newer ones.  I haven't realized that.  I'm thinking about
>> >>>> reverting the change in libvirt or even using heads > 1.  Although
>> >>>> that won't migrate either.  So the only other thing that we can do is
>> >>>> to introduce new parameter, like maxHeads.  The sound you just heard
>> >>>> is me slapping my face because again there will multiple parameters
>> >>>> meaning similar things...
>> >>> Introducing a new parameter is not really viable IMHO, because as you
>> >>> say it'll leave us with two things meaning the same, and will not be
>> >>> compatible with existing apps that know about the current parameter.
>> >>>
>> >>> I think we need to figure out a way to drop the 'heads=1' from any
>> >>> existing configs in /etc/libvirt/qemu when we start up with the new
>> >>> libvirtd for the first time.
>> >>>
>> >> I'm already working on an RFC that would differentiate between loading
>> >> XMLs on daemon start-up and defining them.  The purpose of that is so
>> >> that we can make incompatible XML changes without losing any domains,
>> >> but that's not the point here.  If we were to drop heads='1' anywhere,
>> >> this is the place.  The ABI change would be minimal for the guest.
>> > It isn't sufficient to just differentiate loading on startup
>> > vs defining them IIUC. We need to differentiate loading on
>> > startup from XML previously written by a libvirtd < X.Y.Z
>> > which is why I think we need to have a version number recorded
>> > in the XML ultimately.
>>
>> But a version number isn't sufficient to describe exactly  what the
>> previous libvirt was capable of. Many times new features (externally
>> visible only in the XML) are backported to earlier versions of libvirt
>> downstream (e.g. libvirt-0.10.2 that is used in RHEL6.x and CentOS 6.x),
>> so this still doesn't buy us perfection. Downstream maintainers could
>> make it better by paying very close attention to any use of this version
>> number when backporting new stuff, but there would still be problems if
>> someone decided to build and install an upstream libvirt on a system
>> that previously had some hybrid downstream build.
>>
>> (Not saying we shouldn't do it, just that it's no perfect :-)
>
>Yep, understood. I'm thinking purely in terms of upstream where we do
>not backport features to stable branches. Distros which get into the
>feature backport game have to deal with that pain themselves.
>

How about we use some part of the XML to mark "features" there?  That
part would only be parsed on loading and formatted into migration
cookie and the XML saved on disk.  We can put it into another
namespace.

Each feature would have its handler that "fixes" whatever needs to be
fixed in the postparse part.

>Regards,
>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 :|
On Wed, Jul 22, 2015 at 09:59:00AM +0200, Martin Kletzander wrote:
> On Tue, Jul 21, 2015 at 04:36:45PM +0100, Daniel P. Berrange wrote:
> >On Tue, Jul 21, 2015 at 11:34:08AM -0400, Laine Stump wrote:
> >>On 07/21/2015 09:41 AM, Daniel P. Berrange wrote:
> >>> On Tue, Jul 21, 2015 at 03:35:50PM +0200, Martin Kletzander wrote:
> >>>> On Tue, Jul 21, 2015 at 01:50:22PM +0100, Daniel P. Berrange wrote:
> >>>>> On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
> >>>>>> On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
> >>>>>>> On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
> >>>>>>>> I spend all morning fixing this to be installed properly in the
> >>>>>>>> system.  Anyway, I finally managed to make this work and found out the
> >>>>>>>> guest I used for it is not ready to have multiple monitors.  Anyway,
> >>>>>>>> looking at everything else this definitely works, so ACK, I'll push it
> >>>>>>>> in a while.
> >>>>>>> I'm still a bit worried that all existing guests will have heads='1' in
> >>>>>>> their XML as libvirt is adding that by default, but I don't really see a
> >>>>>>> way around it :-/ We should make sure libvirt stops adding it from now
> >>>>>>> on though ;)
> >>>>>>>
> >>>>>> Well, how would you then limit the outputs to 1?  Having said that, I
> >>>>>> have no idea why we started adding heads="1" automatically and playing
> >>>>>> with different changes, we have another bug in the parsing/formatting
> >>>>>> code.  You'll also won't be able to migrate from older libvirt with
> >>>>>> heads='1' to newer ones.  I haven't realized that.  I'm thinking about
> >>>>>> reverting the change in libvirt or even using heads > 1.  Although
> >>>>>> that won't migrate either.  So the only other thing that we can do is
> >>>>>> to introduce new parameter, like maxHeads.  The sound you just heard
> >>>>>> is me slapping my face because again there will multiple parameters
> >>>>>> meaning similar things...
> >>>>> Introducing a new parameter is not really viable IMHO, because as you
> >>>>> say it'll leave us with two things meaning the same, and will not be
> >>>>> compatible with existing apps that know about the current parameter.
> >>>>>
> >>>>> I think we need to figure out a way to drop the 'heads=1' from any
> >>>>> existing configs in /etc/libvirt/qemu when we start up with the new
> >>>>> libvirtd for the first time.
> >>>>>
> >>>> I'm already working on an RFC that would differentiate between loading
> >>>> XMLs on daemon start-up and defining them.  The purpose of that is so
> >>>> that we can make incompatible XML changes without losing any domains,
> >>>> but that's not the point here.  If we were to drop heads='1' anywhere,
> >>>> this is the place.  The ABI change would be minimal for the guest.
> >>> It isn't sufficient to just differentiate loading on startup
> >>> vs defining them IIUC. We need to differentiate loading on
> >>> startup from XML previously written by a libvirtd < X.Y.Z
> >>> which is why I think we need to have a version number recorded
> >>> in the XML ultimately.
> >>
> >>But a version number isn't sufficient to describe exactly  what the
> >>previous libvirt was capable of. Many times new features (externally
> >>visible only in the XML) are backported to earlier versions of libvirt
> >>downstream (e.g. libvirt-0.10.2 that is used in RHEL6.x and CentOS 6.x),
> >>so this still doesn't buy us perfection. Downstream maintainers could
> >>make it better by paying very close attention to any use of this version
> >>number when backporting new stuff, but there would still be problems if
> >>someone decided to build and install an upstream libvirt on a system
> >>that previously had some hybrid downstream build.
> >>
> >>(Not saying we shouldn't do it, just that it's no perfect :-)
> >
> >Yep, understood. I'm thinking purely in terms of upstream where we do
> >not backport features to stable branches. Distros which get into the
> >feature backport game have to deal with that pain themselves.
> >
> 
> How about we use some part of the XML to mark "features" there?  That
> part would only be parsed on loading and formatted into migration
> cookie and the XML saved on disk.  We can put it into another
> namespace.
> 
> Each feature would have its handler that "fixes" whatever needs to be
> fixed in the postparse part.

I'm not sure what you are suggesting by "features" here, but I'd prefer
if we didn't introduce a chunk of XML which would contain an ever growing
set of hacks. It seems sufficient for us to just record the libvirt
version number which generated the XML document, so newer libvirt can
detect a previous buggy version and correct what's needed, so the
hacks are confined to the code and not spread across code + xml.

Regards,
Daniel
On Wed, Jul 22, 2015 at 10:14:27AM +0100, Daniel P. Berrange wrote:
>I'm not sure what you are suggesting by "features" here, but I'd prefer
>if we didn't introduce a chunk of XML which would contain an ever growing
>set of hacks. It seems sufficient for us to just record the libvirt
>version number which generated the XML document, so newer libvirt can
>detect a previous buggy version and correct what's needed, so the
>hacks are confined to the code and not spread across code + xml.
>

Seems reasonable.  Moreover, the fixes can be handled in a way that
back-porting doesn't cause problems as Laine was worried.  I.e. the XML
would be fixed twice.

>Regards,
>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 :|