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

Re: [libvirt] [PATCH RESENT 07/12] conf: support backend domain name in disk and network devices



Marek Marczykowski wrote:
> At least Xen supports backend drivers in another domain (aka "driver
> domain"). This patch introduces XML config option for such setting as
> 'domain' element with 'name' attribute. Verification its content is left
> for the driver.
>
> In the future some option will be needed for USB devices (hostdev
> objects), but for now libxl doesn't have support for PVUSB.
> ---
>  docs/schemas/domaincommon.rng | 14 ++++++++++++++
>  src/conf/domain_conf.c        | 27 +++++++++++++++++++++++++++
>  src/conf/domain_conf.h        |  2 ++
>  3 files changed, 43 insertions(+)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 8d7e6db..1423187 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -869,6 +869,13 @@
>        </optional>
>        <ref name="target"/>
>        <optional>
> +        <element name="domain">
> +          <attribute name="name">
> +            <ref name="domainName"/>
> +          </attribute>
> +        </element>
> +      </optional>
> +      <optional>
>          <ref name="deviceBoot"/>
>        </optional>
>        <optional>
> @@ -1834,6 +1841,13 @@
>          </element>
>        </optional>
>        <optional>
> +        <element name="domain">
> +          <attribute name="name">
> +            <ref name="domainName"/>
> +          </attribute>
> +        </element>
> +      </optional>
> +      <optional>
>   

I'm certainly no expert in RNG, but do these need to include <empty/>?

BTW, you'll also need to document this in docs/formatdomain.html.in.

>          <element name="model">
>            <attribute name="type">
>              <data type="string">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 257a265..bf1fec6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1103,6 +1103,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
>      VIR_FREE(def->vendor);
>      VIR_FREE(def->product);
>      VIR_FREE(def->script);
> +    VIR_FREE(def->domain_name);
>      if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE)
>          VIR_FREE(def->auth.secret.usage);
>      virStorageEncryptionFree(def->encryption);
> @@ -1228,6 +1229,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
>  
>      VIR_FREE(def->virtPortProfile);
>      VIR_FREE(def->script);
> +    VIR_FREE(def->domain_name);
>      VIR_FREE(def->ifname);
>  
>      virDomainDeviceInfoClear(&def->info);
> @@ -3995,6 +3997,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>      char *vendor = NULL;
>      char *product = NULL;
>      char *script = NULL;
> +    char *domain_name = NULL;
>      int expected_secret_usage = -1;
>      int auth_secret_usage = -1;
>  
> @@ -4153,6 +4156,9 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>              } else if (!script &&
>                         xmlStrEqual(cur->name, BAD_CAST "script")) {
>                  script = virXMLPropString(cur, "path");
> +            } else if (!domain_name &&
> +                       xmlStrEqual(cur->name, BAD_CAST "domain")) {
> +                domain_name = virXMLPropString(cur, "name");
>              } else if (xmlStrEqual(cur->name, BAD_CAST "geometry")) {
>                  if (virXPathUInt("string(./geometry/@cyls)",
>                                   ctxt, &def->geometry.cylinders) < 0) {
> @@ -4447,6 +4453,11 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>          ctxt->node = saved_node;
>      }
>  
> +    if (domain_name != NULL) {
> +        def->domain_name = domain_name;
> +        domain_name = NULL;
> +    }
> +
>   

Is the 'if' necessary here?  It looks like the other fields of def are
unconditionally assigned, e.g.

def->src = source;
source = NULL;

>      if (target == NULL) {
>          virReportError(VIR_ERR_NO_TARGET,
>                         source ? "%s" : NULL, source);
> @@ -4796,6 +4807,7 @@ cleanup:
>      VIR_FREE(vendor);
>      VIR_FREE(product);
>      VIR_FREE(script);
> +    VIR_FREE(domain_name);
>  
>      ctxt->node = save_ctxt;
>      return def;
> @@ -5353,6 +5365,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
>      char *mode = NULL;
>      char *linkstate = NULL;
>      char *addrtype = NULL;
> +    char *domain_name = NULL;
>      virNWFilterHashTablePtr filterparams = NULL;
>      virDomainActualNetDefPtr actual = NULL;
>      xmlNodePtr oldnode = ctxt->node;
> @@ -5451,6 +5464,9 @@ virDomainNetDefParseXML(virCapsPtr caps,
>              } else if (!script &&
>                         xmlStrEqual(cur->name, BAD_CAST "script")) {
>                  script = virXMLPropString(cur, "path");
> +            } else if (!domain_name &&
> +                       xmlStrEqual(cur->name, BAD_CAST "domain")) {
> +                domain_name = virXMLPropString(cur, "name");
>              } else if (xmlStrEqual(cur->name, BAD_CAST "model")) {
>                  model = virXMLPropString(cur, "type");
>              } else if (xmlStrEqual(cur->name, BAD_CAST "driver")) {
> @@ -5682,6 +5698,10 @@ virDomainNetDefParseXML(virCapsPtr caps,
>          def->script = script;
>          script = NULL;
>      }
> +    if (domain_name != NULL) {
> +        def->domain_name = domain_name;
> +        domain_name = NULL;
> +    }
>   

The pattern in this function is to conditionally assign def->foo, so I
guess my above comment is a nit.

>      if (ifname != NULL) {
>          def->ifname = ifname;
>          ifname = NULL;
> @@ -5808,6 +5828,7 @@ cleanup:
>      VIR_FREE(mode);
>      VIR_FREE(linkstate);
>      VIR_FREE(addrtype);
> +    VIR_FREE(domain_name);
>      virNWFilterHashTableFree(filterparams);
>  
>      return def;
> @@ -12909,6 +12930,10 @@ virDomainDiskDefFormat(virBufferPtr buf,
>  
>      virBufferEscapeString(buf, "      <script path='%s'/>\n", def->script);
>  
> +    if (def->domain_name) {
> +        virBufferEscapeString(buf, "      <domain name='%s'/>\n", def->domain_name);
> +    }
> +
>      virDomainDiskGeometryDefFormat(buf, def);
>      virDomainDiskBlockIoDefFormat(buf, def);
>  
> @@ -13456,6 +13481,8 @@ virDomainNetDefFormat(virBufferPtr buf,
>          return -1;
>      virBufferEscapeString(buf, "<script path='%s'/>\n",
>                            def->script);
> +    if (def->domain_name)
> +        virBufferEscapeString(buf, "<domain name='%s'/>\n", def->domain_name);
>      if (def->ifname &&
>          !((flags & VIR_DOMAIN_XML_INACTIVE) &&
>            (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX)))) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index d55d209..db3002b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -669,6 +669,7 @@ struct _virDomainDiskDef {
>      int rawio; /* no = 0, yes = 1 */
>      int sgio; /* enum virDomainDiskSGIO */
>      char *script;
> +    char *domain_name; /* backend domain name */
>  
>      size_t nseclabels;
>      virSecurityDeviceLabelDefPtr *seclabels;
> @@ -919,6 +920,7 @@ struct _virDomainNetDef {
>          unsigned long sndbuf;
>      } tune;
>      char *script;
> +    char *domain_name; /* backend domain name */
>      char *ifname;
>      virDomainDeviceInfo info;
>      char *filter;
>   

Also, with danpb's "improving unit test coverage" mail fresh in my head,
this needs to include a test?  I think changes to domain conf generally
require a one anyhow.  Looks good, but a V2 is needed with the missing
documentation.

Regards,
Jim


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