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

[Jonathon Jongsma]

On Fri, 2019-06-21 at 17:05 +0200, Peter Krempa wrote:
> On Fri, Jun 21, 2019 at 09:53:59 -0500, Jonathon Jongsma wrote:
> > 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.
> 
> I actually meant 'clearing the memory size', thus basically just
> omitting the whole second hunk of your patch. So any previously set
> value would be kept intact.
> 
> Clearing it itself is not a problem for libvirt but might be
> potentially
> unexpected by higher level management apps. But I'm not really sure
> whether't that's a valid case.

I'm starting to wonder if I'm not falling into the trap of only
thinking about things from a qemu/kvm perspective. It looks like a
cirrus device can also be used in the libxl driver, and perhaps it is
configurable under that hypervisor? I don't have any experience there.
In that case, maybe it's safest to simply drop this patch and close the
bug as WONTFIX. Any thoughts?

Jonathon