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@xxxxxxxxxx> > > > > --- > > > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
