Hi Eric, I'm having trouble with the RNG approach. I've created a test rng and test xml with xmllint that can create the problem I am seeing, specifically: test.rng: <?xml version="1.0"?> <grammar xmlns="http://relaxng.org/ns/structure/1.0"; datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes";> <start> <ref name="video"/> </start> <define name="video"> <element name="video"> <optional> <element name="model"> <attribute name="type"> <choice> <value>vga</value> </choice> </attribute> <group> <attribute name="type"> <value>qxl</value> </attribute> <optional> <attribute name="ram"> </attribute> </optional> </group> <optional> <attribute name="vram"> </attribute> </optional> </element> </optional> </element> </define> </grammar> With test.xml being: <video/> $ xmllint --relaxng test.rng test.xml test.rng:9: element element: Relax-NG parser error : Attributes conflicts in group Relax-NG schema test.rng failed to compile <?xml version="1.0"?> <video/> So it seems I cannot have two type instances. I thought maybe you can solve it quicker for me then myself :) ----- Original Message ----- > On 01/17/2013 12:35 PM, Alon Levy wrote: > > Adds a qxl-ram attribute globaly to the video.model element, that > > changes > > s/globaly/globally/ > > > the resulting qemu command line only if video.type == "qxl". > > > > That attribute gets a default value of 64*1024 only if model.type > > is > > "qxl". In effect not changing any xml or argv for non qxl devices. > > > > For qxl devices a new property is set: > > -global qxl-vga.ram_size=<ram>*1024 > > or > > -global qxl.ram_size=<ram>*1024 > > This part of the commit message shows the qemu interface; but it > would > also be handy to show the intended libvirt XML interface. As > written, > this patch is basically adding qxl_ram into: > > <video> > <model type='qxl' vram='65536' qxl-ram='65536' heads='1'/> > </video> > > > > > For the main and secondary qxl devices respectively. > > > > The default for the qxl ram bar is the same as the default for the > > qxl > > vram bar, 64*1024. > > --- > > I've added a qxl-ram attribute. > > Ah, this information is what I was looking for, but because it came > after ---, it would be missing from git history. > > > There is no precedent for adding am attribute > > prefixed like this, so I'm open for any other suggestion on how to > > do it. > > Why prefix it at all? What's wrong with just naming it 'ram', which > is > visually distinct from 'vram'? > > > diff --git a/docs/schemas/domaincommon.rng > > b/docs/schemas/domaincommon.rng > > index 67ae864..50fc834 100644 > > --- a/docs/schemas/domaincommon.rng > > +++ b/docs/schemas/domaincommon.rng > > @@ -2251,7 +2251,9 @@ > > </define> > > <!-- > > A video adapter description, allowing configuration of device > > - model, number of virtual heads, and video ram size > > + model, number of virtual heads, and video ram size. > > + The qxl-ram property is used for qxl types only to specify > > the > > + primary bar size, letting vram specify the secondary bar > > size. > > Michal already pointed out the missing documentation in > docs/formatdomain.html.in. Basically, I would move this comment that > you added here out of the RNG and into the public html.in > documentation. > > Also, in RNG, it is possible to encode the limitation of using the > new > attribute only when tied to type='qxl', as follows (and here, with my > preferred naming of 'ram' instead of 'qxl-ram'): > > <element name="model"> > <choice> > <attribute name="type"> > <choice> > <value>vga</value> > <value>cirrus</value> > <value>vmvga</value> > <value>xen</value> > <value>vbox</value> > </choice> > </attribute> > <group> > <attribute name="type"> > <value>qxl</value> > </attribute> > <optional> > <attribute name="ram"> > <ref name="unsignedInt"/> > </attribute> > </optional> > </group> > </choice> > <optional> > <attribute name="vram"> > <ref name="unsignedInt"/> > </attribute> > </optional> > > > +++ b/src/conf/domain_conf.c > > @@ -7391,6 +7391,7 @@ virDomainVideoDefParseXML(const xmlNodePtr > > node, > > char *type = NULL; > > char *heads = NULL; > > char *vram = NULL; > > + char *qxl_ram = NULL; > > Again, if we go with naming the new attribute just 'ram', this > variable > name can be shorter. > > > @@ -7431,6 +7433,18 @@ virDomainVideoDefParseXML(const xmlNodePtr > > node, > > } > > } > > > > + if (qxl_ram) { > > + if (virStrToLong_ui(qxl_ram, NULL, 10, &def->qxl_ram) < 0) > > { > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > + _("cannot parse video qxl-ram '%s'"), > > qxl_ram); > > + goto error; > > + } > > + } else { > > + if (def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { > > + def->qxl_ram = virDomainVideoDefaultRAM(dom, > > def->type); > > Did you mean for it to always default to the domain default, even > when > vram is present but not the domain default? Or did you want it to > fall > back to the vram value? > > > + } > > + } > > + > > if (vram) { > > Since you documented that '[qxl-]ram' defaults to the value of > 'vram', I > think you need to put the new if block after this existing vram > block. > > > @@ -13383,6 +13398,8 @@ virDomainVideoDefFormat(virBufferPtr buf, > > virBufferAddLit(buf, " <video>\n"); > > virBufferAsprintf(buf, " <model type='%s'", > > model); > > + if (def->qxl_ram && def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) > > + virBufferAsprintf(buf, " qxl-ram='%u'", def->qxl_ram); > > The && def->type == VIR_DOMAIN_VIDEO_TYPE_QXL is not needed here; > def->qxl_ram will be 0 for all other video types, based on the > restrictions you had in the parser. > > > +++ b/src/conf/domain_conf.h > > @@ -1157,6 +1157,7 @@ struct _virDomainVideoAccelDef { > > > > struct _virDomainVideoDef { > > int type; > > + unsigned int qxl_ram; > > unsigned int vram; > > Might be worth adding a comment here on units (that is, both qxl_ram > and > vram use kb). > > > +++ b/src/qemu/qemu_command.c > > @@ -3563,6 +3563,15 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr > > video, > > UINT_MAX / 1024); > > goto error; > > } > > + if (video->qxl_ram > (UINT_MAX / 1024)) { > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > > VIR_ERR_OVERFLOW might be better (but this was copy-and-paste from > vram, > so the same applies there). > > > @@ -6569,23 +6578,48 @@ qemuBuildCommandLine(virConnectPtr conn, > > virCommandAddArgList(cmd, "-vga", vgastr, NULL); > > > > if (def->videos[0]->type == > > VIR_DOMAIN_VIDEO_TYPE_QXL) { > > - if (def->videos[0]->vram && > > + if ((def->videos[0]->vram || > > def->videos[0]->qxl_ram) && > > qemuCapsGet(caps, QEMU_CAPS_DEVICE)) { > > - if (def->videos[0]->vram > (UINT_MAX / > > 1024)) { > > + int qxl_ram = def->videos[0]->qxl_ram; > > + int vram = def->videos[0]->vram; > > + if (vram > (UINT_MAX / 1024)) { > > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > > More places where the existing code would be better as > VIR_ERR_OVERFLOW. > > > + if (qemuCapsGet(caps, > > QEMU_CAPS_DEVICE_QXL_VGA)) { > > + if (qxl_ram) { > > + virCommandAddArg(cmd, "-global"); > > + virCommandAddArgFormat(cmd, > > + > > "qxl-vga.ram_size=%u", > > + qxl_ram * > > 1024); > > + } > > + if (vram) { > > + virCommandAddArg(cmd, "-global"); > > + virCommandAddArgFormat(cmd, > > + > > "qxl-vga.vram_size=%u", > > + vram * > > 1024); > > + } > > + } else { > > + if (qxl_ram) { > > + virCommandAddArg(cmd, "-global"); > > + virCommandAddArgFormat(cmd, > > "qxl.ram_size=%u", > > + qxl_ram * > > 1024); > > + } > > + if (vram) { > > + virCommandAddArg(cmd, "-global"); > > + virCommandAddArgFormat(cmd, > > "qxl.vram_size=%u", > > + vram * > > 1024); > > + } > > + } > > This feels long. Why not the shorter: > > const char *dev = (qemuCapsGet(caps, QEMU_CAPS_DEVICE_QXL_VGA) > ? "qxl-vga" : "qxl"); > if (qxl_ram) > virCommandAddArgFormat(cmd, "-global=%s.ram_size=%u", > dev, qxl_ram * 1024); > if (vram) > virCommandAddArgFormat(cmd, "-global=%s.vram_size=%u", > dev, vram * 1024); > > > > +++ > > b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-qxl-vga.args > > @@ -3,5 +3,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test > > LOGNAME=test QEMU_AUDIO_DRV=spice \ > > unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda \ > > /dev/HostVG/QEMUGuest1 -spice > > port=5903,tls-port=5904,addr=127.0.0.1,\ > > x509-dir=/etc/pki/libvirt-spice,tls-channel=main,plaintext-channel=inputs > > -vga \ > > -qxl -global qxl-vga.vram_size=33554432 -device > > qxl,id=video1,vram_size=67108864,bus=pci.0,addr=0x4 \ > > +qxl -global qxl-vga.ram_size=67108864 -global > > qxl-vga.vram_size=33554432 -device > > qxl,id=video1,ram_size=67108864,vram_size=67108864,bus=pci.0,addr=0x4 > > \ > > This is a really long line; please insert a \-newline pair to break > it > and keep the file back under 80 columns. > > Looking forward to v2. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list