[libvirt] [PATCH] domain_conf: Use virDomainParseMemory more widely

[Jiri Denemark]

On Fri, Nov 07, 2014 at 15:30:10 +0100, Michal Privoznik wrote:
> As reviewing patches upstream it occurred to me, that we have two
> functions doing nearly the same: virDomainParseMemory which
> expects XML in the following format:
> 
>   <memory unit='MiB'>1337</memory>
> 
> The other function being virDomainHugepagesParseXML expecting the
> following format:
> 
>   <someElement memory='1337' unit='MiB'/>

This should rather be

    <someElement size=.../>

to match what the code really does.

> It wouldn't matter to have two functions handle two different
> scenarios like this if we could only not copy code that handles
> 32bit arches around. So this code merges the common parts into
> one by inventing new @units_xpath argument to
> virDomainParseMemory which allows overriding the default location
> of @unit attribute in XML. With this change both scenarios above
> can be parsed with virDomainParseMemory. Sweet. Of course,
> everything is commented as it should be.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/conf/domain_conf.c | 137 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 80 insertions(+), 57 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 21309b0..e097af7 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6318,14 +6318,30 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
>      goto cleanup;
>  }
>  
> -
> -/* Parse a value located at XPATH within CTXT, and store the
> - * result into val.  If REQUIRED, then the value must exist;
> - * otherwise, the value is optional.  The value is in bytes.
> - * Return 1 on success, 0 if the value was not present and
> - * is not REQUIRED, -1 on failure after issuing error. */
> +/**
> + * virDomainParseScaledValue:
> + * @bytes_xpath: XPath to memory amount

I think the "bytes_xpath" name is slightly misleading and plain "xpath"
would be better since this is a general function and we may be parsing
something completely different not to mention that the value may
represent for example Mebibytes depending on what is parsed from
@units_xpath.

> + * @units_xpath: XPath to units attribute
> + * @ctxt: XPath context
> + * @val: scaled value is stored here
> + * @scale: default scale for @val
> + * @max: maximal @val allowed
> + * @required: is the value required?
> + *
> + * Parse a value located at @bytes_xpath within @ctxt, and store
> + * the result into @val. By default, if @units_xpath is NULL the
> + * unit attribute must be an attribute to @bytes_xpath.
> + * Otherwise, the unit attribute is looked for under specified
> + * path. If @required is set, then the value must exist;
> + * otherwise, the value is optional. The value is in bytes.
> + *
> + * Returns 1 on success,
> + *         0 if the value was not present and !@required,
> + *         -1 on failure after issuing error.
> + */
>  static int
> -virDomainParseScaledValue(const char *xpath,
> +virDomainParseScaledValue(const char *bytes_xpath,
> +                          const char *units_xpath,
>                            xmlXPathContextPtr ctxt,
>                            unsigned long long *val,
>                            unsigned long long scale,
...
> @@ -6379,17 +6398,35 @@ virDomainParseScaledValue(const char *xpath,
>  }
>  
>  
> -/* Parse a memory element located at XPATH within CTXT, and store the
> - * result into MEM, in blocks of 1024.  If REQUIRED, then the value
> - * must exist; otherwise, the value is optional.  The value must not
> - * exceed VIR_DOMAIN_MEMORY_PARAM_UNLIMITED once scaled; additionally,
> - * if CAPPED is true, the value must fit within an unsigned long (only
> - * matters on 32-bit platforms).
> +/**
> + * virDomainParseMemory:
> + * @bytes_xpath: XPath to memory amount

My comment about "bytes_xpath" above is applicable here as well.
Although we at least know the value has to do something with bytes.

> + * @units_xpath: XPath to units attribute
> + * @ctxt: XPath context
> + * @mem: scaled memory amount is stored here
> + * @required: whether value is required
> + * @capped: whether scaled value must fit within unsigned long
>   *
> - * Return 0 on success, -1 on failure after issuing error.  */
> + * Parse a memory element or attribute located at @bytes_xpath
> + * within @ctxt, and store the result into @mem, in blocks of
> + * 1024. By default, if @units_xpath is NULL the unit attribute
> + * must be an attribute to @bytes_xpath. Otherwise, the unit
> + * attribute is looked for under specified path. If @required is
> + * set, then the value must exist; otherwise, the value is
> + * optional.  The value must not exceed
> + * VIR_DOMAIN_MEMORY_PARAM_UNLIMITED once scaled; additionally,
> + * if @capped is true, the value must fit within an unsigned long
> + * (only matters on 32-bit platforms).
> + *
> + * Return 0 on success, -1 on failure after issuing error.
> + */
>  static int
> -virDomainParseMemory(const char *xpath, xmlXPathContextPtr ctxt,
> -                     unsigned long long *mem, bool required, bool capped)
> +virDomainParseMemory(const char *bytes_xpath,
> +                     const char *units_xpath,
> +                     xmlXPathContextPtr ctxt,
> +                     unsigned long long *mem,
> +                     bool required,
> +                     bool capped)
>  {
>      int ret = -1;
>      unsigned long long bytes, max;
...


ACK

Jirka