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