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

[Jonathon Jongsma]

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 at redhat.com>
> > ---
> > 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