Re: [libvirt] [PATCH] Don't parse/format vram attribute for cirrus video

On Fri, 2019-06-21 at 08:13 +0200, Peter Krempa wrote: > On Thu, Jun 20, 2019 at 14:51:12 -0500, Jonathon Jongsma wrote: > > Since the cirrus vga memory size isn't configurable, we can ignore > > any > > 'vram' attribute when parsing a domain definition. However, when no > > value is specified, it ends up getting set to a default value of > > 16MB. > > This 16MB value is not used anywhere (for example, it is not passed > > as > > an argument to qemu), but is displayed in the XML definition. So by > > changing this default value to 0, it will also be omitted from the > > XML > > definition of the domain. > > > > Fixes: rhbz#1447831 > > Please always use the full link so that it's clickable and users > don't > have to figure out what 'rhbz' is. > > > Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; > > --- > > This is an attempt to apply the fix suggested by Gerd at > > https://bugzilla.redhat.com/show_bug.cgi?id=1447831#c2. I'm not > > totally confident that this is the right approach, since I'm > > relatively new to the code. Another approach might be to simply > > close > > the bug as NOTABUG since it doesn't seem that having this unused > > attribute in the domain definition has any significant drawbacks. > > We certainly should not set any default if it's not used. There's not > much else we can do though as we did put a default into the > configuration. I'm not sure I understand what you're suggesting here. This sentence seems to imply that you think we should *not* set a default, but down below you say that you're not certain whether we should clear the default? It seems a bit contradictory, but perhaps I'm simply misunderstanding.

> Doing any validation would mean that any VM which had the > default errorneously configured in the XML would fail to start. Yeah, I did not do any 'validation' of cirrus vram attributes because I didn't want existing VMs to fail to start. And all existing VMs that use the cirrus device presumably have this attribute set in their XML definition. So I simply ignored this parameter rather than rejecting it. But as I said, I'm not sure whether that's the correct approach for libvirt.

> > Whether it's worth clearing the default or not I'm not so certain but > I > don't think it should hurt. > > You definitely should fix the docs though: > > https://libvirt.org/formatdomain.html#elementsVideo > > As it says 'For a guest of type "kvm", the default video is: type > with > value "cirrus", vram with value "16384" and heads with value "1".' > > (see docs/formatdomain.html.in ) Thanks, will do. Jonathon