[libvirt] [PATCH v4] qemu: Use heads parameter for QXL driver
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. 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); -- 2.1.0
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=567161fdd47aeb6987e700702f6bb... 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=567161fdd47aeb6987e700702f6bb...
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=567161fdd47aeb6987e700702f6bb...
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=567161fdd47aeb6987e700702f6bb...
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=567161fdd47aeb6987e700702f6bb...
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 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 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 -- |: 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 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 -- |: 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 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:
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
On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote: 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 -- |: 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 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 -- |: 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 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 :|
participants (6)
-
Christophe Fergeau -
Daniel P. Berrange -
Frediano Ziglio -
Laine Stump -
Martin Kletzander -
Peter Krempa