Re: [libvirt] [PATCH v4 6/6] qemu-command: introduce new vgamem attribute for QXL video device
On 12/01/2014 01:50 PM, Giuseppe Scrivano wrote:
Pavel Hrdina <phrdina(a)redhat.com> writes:
> The vgamem is a new attribute for QXL. The vram stil exists and its
> only silently updated to proper value. The configuration with vram=9216
> is wrong and should be updated in virt-manager tests.
the issue exists when the type of an existing QXL device is changed.
virt-manager re-uses the XML configuration without modifying attributes
it doesn't understand (as vgamem in this case) but changing only
type='qxl' to type='$NEW_TYPE'.
My doubt here is if an additional attribute should be silently ignored
or users of the API should be filtering it out if type != 'qxl'.
Another point is that this attribute will be added in future for some
other models, then all users must be updated as well to support it,
instead of having it only in libvirt.
What do you think?
Regards,
Giuseppe
Well, I think that this is not correct usage of the libvirt XML. You
should not just change one part of the device configuration. If user of
virt-manager change the device type, you should create a new XML
according to the new device type and copy all valid values and then drop
the original XML configuration.
I don't think that just silently remove unsupported configuration is
appropriate solution. Even if we will print warning that it's not
supported and it will be removed I still don't like it.
It's not that the user should filter the "vgamem" out if type !=
'qxl'
but the user of the XML configuration should have some templates for
existing devices and should use for each type different template. With
this approach is ease to manage the device type change and also new
features introduced by libvirt. You just modify the affected templates
to support new features.
Pavel