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

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



On 11/07/2014 03:30 PM, 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'/>
> 
> 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.

There's no need to commend yourself for your comments in the commit message :)

See below for a few nit picks.

> Signed-off-by: Michal Privoznik <mprivozn 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
> + * @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.

The attribute doesn't have to be present.
It would also be nice to mention the scaling:
The value is scaled by units located at @units_xpath (or the 'unit' attribute
under @bytes_xpath if @units_xpath is NULL). If units are not present,
the default @scale is used.

> + * Otherwise, the unit attribute is looked for under specified
> + * path.

This sentence seems redundant.

> If @required is set, then the value must exist;
> + * otherwise, the value is optional. The value is in bytes.

To distinguish it from the input value (although their existences are linked
together):
The resulting value is in bytes.

Jan

Attachment: signature.asc
Description: OpenPGP digital signature


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