[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] conf: add video driver configuration element




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 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;
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]