[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



  John Ferlan wrote:

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

Thanks, I'll rebase the change.

Also, looking at f5384fb4, it looks like I can still reuse the <driver>
subelement because it seems it goes in line with the vga-related
configuration I'm adding.

> 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='?'.../>"

Yeah, I was planning to update the doc as a separate commit after this
one gets merged because I wasn't sure this schema change will go in as
is, so I don't have to re-roll the doc patches as well (yeah, I'm
lazy...)

> > 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"?

Will fix this one.

> >  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) {
> 
> <= ?

Hm, 0 seems to correspond to the first element in the enum, so "<="
should be valid. I'll double check.

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

Yeah, that'll need to be fixed if it stays relevant after rebase.

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

There's at least one thing I'd like to make controllable using this
<driver> element: screen resolution (width and height in pixels).

There's one more thing in bhyve that I'd like add support for: it
supports the 'wait' argument for video device so VM will start booting
only after clients connect to it via VNC. I'm not if this one fits the
video <driver> element very well though (probably should be part of
<graphics>?).

> 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

Actually, bhyve only supports 'gop', so I'm targeting this one
specifically. Though I guess it could be used for type='vga' as well
because there's nothing bhyve specific in 'vgaconf' as I can see.

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

Roman Bogorodskiy

Attachment: signature.asc
Description: PGP signature


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