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