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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list