[libvirt] [PATCH v1 05/15] conf: Introduce VIR_DOMAIN_LOADER_TYPE_NONE

[Laszlo Ersek]

On 02/27/19 11:04, Michal Privoznik wrote:
> This is going to extend virDomainLoader enum. The reason is that
> once loader path is NULL its type makes no sense. However, since
> value of zero corresponds to VIR_DOMAIN_LOADER_TYPE_ROM the
> following XML would be produced:
> 
>   <os>
>     <loader type='rom'/>
>     ...
>   </os>
> 
> To solve this, introduce VIR_DOMAIN_LOADER_TYPE_NONE which would
> correspond to value of zero and then use post parse callback to
> set the default loader type to 'rom' if needed.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/conf/domain_conf.c              | 23 +++++++++++++++++++++--
>  src/conf/domain_conf.h              |  3 ++-
>  src/qemu/qemu_command.c             |  1 +
>  src/qemu/qemu_domain.c              |  1 +
>  tests/domaincapsschemadata/full.xml |  1 +
>  5 files changed, 26 insertions(+), 3 deletions(-)

Sounds pretty complex, but I guess it makes sense. If both @type and the
pathname content are missing, then @type will default to NONE. (This
used not to be possible, given that pathname used to be required.) If
@type is absent but the pathname is present, then we flip the type
manually to ROM.

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f622a4dddf..b436b91c66 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1013,6 +1013,7 @@ VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST,
>  
>  VIR_ENUM_IMPL(virDomainLoader,
>                VIR_DOMAIN_LOADER_TYPE_LAST,
> +              "none",
>                "rom",
>                "pflash",
>  );
> @@ -4286,6 +4287,20 @@ virDomainDefPostParseMemory(virDomainDefPtr def,
>  }
>  
>  
> +static void
> +virDomainDefPostParseOs(virDomainDefPtr def)
> +{
> +    if (!def->os.loader)
> +        return;
> +
> +    if (def->os.loader->path &&
> +        def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_NONE) {
> +        /* By default, loader is type of 'rom' */
> +        def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
> +    }
> +}
> +
> +
>  static void
>  virDomainDefPostParseMemtune(virDomainDefPtr def)
>  {
> @@ -5465,6 +5480,8 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
>      if (virDomainDefPostParseMemory(def, data->parseFlags) < 0)
>          return -1;
>  
> +    virDomainDefPostParseOs(def);
> +
>      virDomainDefPostParseMemtune(def);
>  
>      if (virDomainDefRejectDuplicateControllers(def) < 0)
> @@ -18706,7 +18723,7 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>  
>      if (type_str) {
>          int type;
> -        if ((type = virDomainLoaderTypeFromString(type_str)) < 0) {
> +        if ((type = virDomainLoaderTypeFromString(type_str)) <= 0) {

Why is this change necessary? Hm... I assume, due to the enum
auto-generation in libvirt, the introduction of "none" will
automatically cause virDomainLoaderTypeFromString() to recognize "none"
as value 0. But we want to act like "none" isn't a valid value (it's
internal use only). Hence the same error message as before.

I reckon this is OK.

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

Thanks
Laszlo

>              virReportError(VIR_ERR_XML_DETAIL,
>                             _("unknown type value: %s"), type_str);
>              goto cleanup;
> @@ -27611,12 +27628,14 @@ virDomainLoaderDefFormat(virBufferPtr buf,
>      if (loader->secure)
>          virBufferAsprintf(buf, " secure='%s'", secure);
>  
> -    virBufferAsprintf(buf, " type='%s'", type);
> +    if (loader->type != VIR_DOMAIN_LOADER_TYPE_NONE)
> +        virBufferAsprintf(buf, " type='%s'", type);
>  
>      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);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 1f8454b38c..4e8c02b5e3 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1899,7 +1899,8 @@ struct _virDomainBIOSDef {
>  };
>  
>  typedef enum {
> -    VIR_DOMAIN_LOADER_TYPE_ROM = 0,
> +    VIR_DOMAIN_LOADER_TYPE_NONE = 0,
> +    VIR_DOMAIN_LOADER_TYPE_ROM,
>      VIR_DOMAIN_LOADER_TYPE_PFLASH,
>  
>      VIR_DOMAIN_LOADER_TYPE_LAST
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 74f34af292..92729485ff 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9846,6 +9846,7 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
>          }
>          break;
>  
> +    case VIR_DOMAIN_LOADER_TYPE_NONE:
>      case VIR_DOMAIN_LOADER_TYPE_LAST:
>          /* nada */
>          break;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index cf7e650b34..cc3a01397c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -12201,6 +12201,7 @@ qemuDomainSetupLoader(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
>                  goto cleanup;
>              break;
>  
> +        case VIR_DOMAIN_LOADER_TYPE_NONE:
>          case VIR_DOMAIN_LOADER_TYPE_LAST:
>              break;
>          }
> diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml
> index eafba1ae5b..0a46e6bb78 100644
> --- a/tests/domaincapsschemadata/full.xml
> +++ b/tests/domaincapsschemadata/full.xml
> @@ -10,6 +10,7 @@
>        <value>/foo/bar</value>
>        <value>/tmp/my_path</value>
>        <enum name='type'>
> +        <value>none</value>
>          <value>rom</value>
>          <value>pflash</value>
>        </enum>
>