[libvirt] [PATCH 1/2] conf: add video driver configuration element
John Ferlan
jferlan at redhat.com
Mon Jun 12 12:32:16 UTC 2017
On 05/09/2017 07:53 AM, Roman Bogorodskiy wrote:
> Add support for video driver configuration. In domain xml it looks like
> this:
>
> <video>
> <model type=''>
> <driver .../>
> </model>
> </video>
>
> At present, the only supported configuration is 'vgaconf' that looks this way:
>
> <driver vgaconf='io|on|off'>
>
> It was added with bhyve gop video in mind to allow users control how the
> video device is exposed to the guest, specifically, how VGA I/O is
> handled.
>
> Signed-off-by: Roman Bogorodskiy <bogorodskiy at gmail.com>
> ---
> docs/schemas/domaincommon.rng | 13 +++++++++
> src/conf/domain_conf.c | 63 +++++++++++++++++++++++++++++++++++++++++--
> src/conf/domain_conf.h | 17 ++++++++++++
> src/libvirt_private.syms | 2 ++
> 4 files changed, 93 insertions(+), 2 deletions(-)
>
Due to languishing on list and given jtomko's changes in a similar
place, this no longer 'git am -3' applies... Could you please rebase
and resend? You may also need to rethink the <driver> subelement too as
it seems jtomko's commit f5384fb4 added this as well.
The xml2xml test should also be added here w/ various options...
There's no adjustment to the formatdomain.html.in to describe this -
would it be strictly related to one specific "<model type='?'.../>"
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 281309ec0..f45820383 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3278,6 +3278,19 @@
> </optional>
> </element>
> </optional>
> + <optional>
> + <element name="driver">
> + <optional>
> + <attribute name="vgaconf">
> + <choice>
> + <value>io</value>
> + <value>on</value>
> + <value>off</value>
> + </choice>
> + </attribute>
> + </optional>
Seems strange to have an optional <driver> with one optional attribute
"vgaconf".
> + </element>
> + </optional>
> </element>
> </optional>
> <optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0ff216e3a..2ab55b52f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -553,6 +553,11 @@ VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
> "virtio",
> "gop")
>
> +VIR_ENUM_IMPL(virDomainVideoVgaconf, VIR_DOMAIN_VIDEO_VGACONF_LAST,
> + "io",
> + "on",
> + "off")
> +
A "nit", but should it be "VGA" and not "Vga"?
> VIR_ENUM_IMPL(virDomainInput, VIR_DOMAIN_INPUT_TYPE_LAST,
> "mouse",
> "tablet",
> @@ -2280,6 +2285,7 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def)
> virDomainDeviceInfoClear(&def->info);
>
> VIR_FREE(def->accel);
> + VIR_FREE(def->driver);
> VIR_FREE(def);
> }
>
> @@ -13368,6 +13374,43 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
> return def;
> }
>
> +static virDomainVideoDriverDefPtr
> +virDomainVideoDriverDefParseXML(xmlNodePtr node)
> +{
> + xmlNodePtr cur;
> + virDomainVideoDriverDefPtr def;
> + char *vgaconf = NULL;
> + int val;
> +
> + cur = node->children;
> + while (cur != NULL) {
> + if (cur->type == XML_ELEMENT_NODE) {
> + if (!vgaconf &&
> + xmlStrEqual(cur->name, BAD_CAST "driver")) {
> + vgaconf = virXMLPropString(cur, "vgaconf");
> + }
> + }
> + cur = cur->next;
> + }
> +
> + if (!vgaconf)
> + return NULL;
> +
> + if (VIR_ALLOC(def) < 0)
> + goto cleanup;
> +
> + if ((val = virDomainVideoVgaconfTypeFromString(vgaconf)) <= 0) {
<= ?
How does VIR_DOMAIN_VIDEO_VGACONF_IO get set? Perhaps a specific xml2xml
test...
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown vgaconf value '%s'"), vgaconf);
> + goto cleanup;
> + }
> + def->vgaconf = val;
> +
> + cleanup:
> + VIR_FREE(vgaconf);
> + return def;
> +}
> +
> static virDomainVideoDefPtr
> virDomainVideoDefParseXML(xmlNodePtr node,
> const virDomainDef *dom,
> @@ -13405,6 +13448,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
> }
>
> def->accel = virDomainVideoAccelDefParseXML(cur);
> + def->driver = virDomainVideoDriverDefParseXML(cur);
> }
> }
> cur = cur->next;
> @@ -22998,6 +23042,18 @@ virDomainVideoAccelDefFormat(virBufferPtr buf,
> virBufferAddLit(buf, "/>\n");
> }
>
> +static void
> +virDomainVideoDriverDefFormat(virBufferPtr buf,
> + virDomainVideoDriverDefPtr def)
> +{
> + virBufferAddLit(buf, "<driver");
> + if (def->vgaconf) {
> + virBufferAsprintf(buf, " vgaconf='%s'",
> + virDomainVideoVgaconfTypeToString(def->vgaconf));
> + }
Without def->driver, the def->vgaconf doesn't exist, true?
If !vgaconf, then all you're getting is <driver>, which while
technically legal according to the RNG added here does look a little
strange. Are there plans to "enhance" this in the future? Always a bit
difficult to take that crystal ball out, but wouldn't that type of
enhancement need some sort of "vgaconf" or would it be possible for
something else...
Given what I found w/ jtomko's changes and this, maybe the optional
subelement just becomes <vgaconf>, unless you see <driver> being
expanded. Still vgaconf would seem to be related to model type='vga' in
my mind at least...
John
> + virBufferAddLit(buf, "/>\n");
> +}
> +
>
> static int
> virDomainVideoDefFormat(virBufferPtr buf,
> @@ -23028,10 +23084,13 @@ virDomainVideoDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, " heads='%u'", def->heads);
> if (def->primary)
> virBufferAddLit(buf, " primary='yes'");
> - if (def->accel) {
> + if (def->accel || def->driver) {
> virBufferAddLit(buf, ">\n");
> virBufferAdjustIndent(buf, 2);
> - virDomainVideoAccelDefFormat(buf, def->accel);
> + if (def->accel)
> + virDomainVideoAccelDefFormat(buf, def->accel);
> + if (def->driver)
> + virDomainVideoDriverDefFormat(buf, def->driver);
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</model>\n");
> } else {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 09fb7aada..cbf25a67b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1350,6 +1350,16 @@ typedef enum {
> } virDomainVideoType;
>
>
> +typedef enum {
> + VIR_DOMAIN_VIDEO_VGACONF_IO = 0,
> + VIR_DOMAIN_VIDEO_VGACONF_ON,
> + VIR_DOMAIN_VIDEO_VGACONF_OFF,
> +
> + VIR_DOMAIN_VIDEO_VGACONF_LAST
> +} virDomainVideoVgaconf;
> +
> +VIR_ENUM_DECL(virDomainVideoVgaconf)
> +
> typedef struct _virDomainVideoAccelDef virDomainVideoAccelDef;
> typedef virDomainVideoAccelDef *virDomainVideoAccelDefPtr;
> struct _virDomainVideoAccelDef {
> @@ -1358,6 +1368,12 @@ struct _virDomainVideoAccelDef {
> };
>
>
> +typedef struct _virDomainVideoDriverDef virDomainVideoDriverDef;
> +typedef virDomainVideoDriverDef *virDomainVideoDriverDefPtr;
> +struct _virDomainVideoDriverDef {
> + virDomainVideoVgaconf vgaconf;
> +};
> +
> struct _virDomainVideoDef {
> int type;
> unsigned int ram; /* kibibytes (multiples of 1024) */
> @@ -1367,6 +1383,7 @@ struct _virDomainVideoDef {
> unsigned int heads;
> bool primary;
> virDomainVideoAccelDefPtr accel;
> + virDomainVideoDriverDefPtr driver;
> virDomainDeviceInfo info;
> };
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e6901a8f1..40995533c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -525,6 +525,8 @@ virDomainVideoDefaultType;
> virDomainVideoDefFree;
> virDomainVideoTypeFromString;
> virDomainVideoTypeToString;
> +virDomainVideoVgaconfTypeFromString;
> +virDomainVideoVgaconfTypeToString;
> virDomainVirtTypeFromString;
> virDomainVirtTypeToString;
> virDomainWatchdogActionTypeFromString;
>
More information about the libvir-list
mailing list