Re: [libvirt] [PATCH V2 2/6] qemu: Introduce vgamem attribute for video model
On 2014/7/24 17:40, Michal Privoznik wrote:
> ---
> src/conf/domain_conf.c | 48 ++++++++++++++++++++++++++++++-
> src/conf/domain_conf.h | 3 +-
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_command.c | 75 ++++++++++++++++++++++++++++++++----------------
> 4 files changed, 101 insertions(+), 26 deletions(-)
missing docs and RNG schema adjustment. And I'd introduce a new XML to test too.
Thank you for your review.
docs and RNG schema are in patch 6/6. Do you mean that docs and source code
should be merged into one patch or I missed some other docs?
>
> + if (vgamem) {
> + if (def->type != VIR_DOMAIN_VIDEO_TYPE_VGA &&
> + def->type != VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
> + def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("vgamem attribute only supported "
> + "for type of vga, vmvga and qxl"));
> + goto error;
> + }
Spaces at EOL
Sorry. "make syntax-check" shows me the nits.
> + if (virStrToLong_ui(vgamem, NULL, 10,
&def->vgamem) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("cannot parse video vgamem '%s'"),
vgamem);
> + goto error;
> + }
> + } else {
> + def->vgamem = virDomainVideoDefaultVgamem(def->type);
> + }
> +
And again.
But I'm wondering how's vgamem different to vram. If it covers the same attribute
but for different models, I'd vote for keeping already existing attribute name.
Qxl model has attribute vram_size/vram_size_mb and vgamem_mb in QEMU.
Vga model has attribute vgamem_mb in QEMU but not vram_size/vram_size_mb.
So I use a new attribute vgamem in libvirt to make variable/attribute
name be consistent with QEMU's.
IIUC, attributes in libvirt and qemu for different models are shown as below.
Before:
model libvirt-attribute(xml) qemu-attribute
qxl vram vram_size
qxl no attribute vgamem_mb
vga vram libvirt passes no attribute to qemu; and qemu has
no attribute named like vram*
vga no attribute vgamem_mb
after:
model libvirt-attribute(xml) qemu-attribute
qxl vram vram_size
qxl vgamem vgamem_mb
vga vram libvirt passes no attribute to qemu; and qemu has
no attribute named like vram*
vga vgamem vgamem_mb
And so do other models.(I hope my expression is clear.)
I think it's less confusion to introduce new attribute vgamem.
Gerd Hoffmann's suggestion is almost the same.
https://www.redhat.com/archives/libvir-list/2014-June/msg00569.html