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

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(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. 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.