[libvirt] [PATCH v1 04/15] virDomainLoaderDefParseXML: Allow loader path to be NULL

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> Except not really. At least for now.
> 
> In the future, the firmware will be selected automagically.
> Therefore, it makes no sense to require path to one in the domain

I suggest replacing "path to one" with "the pathname of a specific
firmware binary". "One" in the above context is meaningful, but it takes
some mental gymnastics to resolve.

> XML. But since it is not implemented do not really allow the path
> to be NULL. Only move code around to prepare it for further
> expansion.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  docs/schemas/domaincommon.rng |  4 +++-
>  src/conf/domain_conf.c        | 29 +++++++++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 5345e54342..bebe39de76 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -283,7 +283,9 @@
>                  </choice>
>                </attribute>
>              </optional>
> -            <ref name="absFilePath"/>
> +            <optional>
> +              <ref name="absFilePath"/>
> +            </optional>
>            </element>
>          </optional>
>          <optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 477deb777e..f622a4dddf 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6564,6 +6564,22 @@ virDomainDefMemtuneValidate(const virDomainDef *def)
>  }
>  
>  
> +static int
> +virDomainDefOSValidate(const virDomainDef *def)
> +{
> +    if (!def->os.loader)
> +        return 0;
> +
> +    if (!def->os.loader->path) {
> +        virReportError(VIR_ERR_XML_DETAIL, "%s",
> +                       _("no loader path specified"));
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  static int
>  virDomainDefValidateInternal(const virDomainDef *def)
>  {
> @@ -6602,6 +6618,9 @@ virDomainDefValidateInternal(const virDomainDef *def)
>      if (virDomainDefMemtuneValidate(def) < 0)
>          return -1;
>  
> +    if (virDomainDefOSValidate(def) < 0)
> +        return -1;
> +
>      return 0;
>  }
>  
> @@ -18668,6 +18687,9 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>      type_str = virXMLPropString(node, "type");
>      loader->path = (char *) xmlNodeGetContent(node);
>  
> +    if (STREQ_NULLABLE(loader->path, ""))
> +        VIR_FREE(loader->path);
> +
>      if (readonly_str &&
>          (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
>          virReportError(VIR_ERR_XML_DETAIL,
> @@ -27589,9 +27611,12 @@ virDomainLoaderDefFormat(virBufferPtr buf,
>      if (loader->secure)
>          virBufferAsprintf(buf, " secure='%s'", secure);
>  
> -    virBufferAsprintf(buf, " type='%s'>", type);
> +    virBufferAsprintf(buf, " type='%s'", type);
>  
> -    virBufferEscapeString(buf, "%s</loader>\n", loader->path);
> +    if (loader->path)
> +        virBufferEscapeString(buf, ">%s</loader>\n", loader->path);
> +    else
> +        virBufferAddLit(buf, "/>\n");
>      if (loader->nvram || loader->templt) {
>          virBufferAddLit(buf, "<nvram");
>          virBufferEscapeString(buf, " template='%s'", loader->templt);
> 

I think this patch does what it says on the tin, but I'm not sure myself
if "what it says on the tin" is the right thing to do. (In other words,
whether the syntax proposed in the blurb is indeed our end-goal.) I
don't doubt it, I just don't have an opinion. So I'll defer to Dan on
this patch. Technically, it looks OK to me.

With the commit message update:

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Thanks
Laszlo