[libvirt] [PATCHv2 2/3] conf: tighten up XML integer parsing

[Laine Stump]

On 04/19/2012 02:24 PM, Eric Blake wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=617711 reported that
> even with my recent patched to allow <memory unit='G'>1</memory>,
> people can still get away with trying <memory>1G</memory> and
> silently get <memory unit='KiB'>1</memory> instead.  While
> virt-xml-validate catches the error, our C parser did not.
>
> Not to mention that it's always fun to fix bugs while reducing
> lines of code.  :)
>
> * src/conf/domain_conf.c (virDomainParseMemory): Check for parse error.
> (virDomainDefParseXML): Avoid strtoll.
> * src/conf/storage_conf.c (virStorageDefParsePerms): Likewise.
> * src/util/xml.c (virXPathLongBase, virXPathULongBase)
> (virXPathULongLong, virXPathLongLong): Likewise.
> ---
>
> v2: fix virDomainParseMemory to detect parse errors on optional
> arguments, rather than silently ignoring those arguments
>
>  src/conf/domain_conf.c  |   24 ++++++++++++++----------
>  src/conf/storage_conf.c |    7 ++++---
>  src/util/xml.c          |   36 ++++--------------------------------
>  3 files changed, 22 insertions(+), 45 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5ab052a..8f352ea 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7609,10 +7609,16 @@ virDomainParseMemory(const char *xpath, xmlXPathContextPtr ctxt,
>          virReportOOMError();
>          goto cleanup;
>      }
> -    if (virXPathULongLong(xpath_full, ctxt, &bytes) < 0) {
> -        if (required)
> +    ret = virXPathULongLong(xpath_full, ctxt, &bytes);
> +    if (ret < 0) {
> +        if (ret == -2)
>              virDomainReportError(VIR_ERR_XML_ERROR,
> -                                 "%s", _("missing memory element"));
> +                                 _("could not parse memory element %s"),
> +                                 xpath);
> +        else if (required)
> +            virDomainReportError(VIR_ERR_XML_ERROR,
> +                                 _("missing memory element %s"),
> +                                 xpath);
>          else
>              ret = 0;
>          goto cleanup;
> @@ -8086,12 +8092,11 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>              if (STREQ(tmp, "reset")) {
>                  def->clock.data.utc_reset = true;
>              } else {
> -                char *conv = NULL;
> -                unsigned long long val;
> -                val = strtoll(tmp, &conv, 10);
> -                if (conv == tmp || *conv != '\0') {
> -                    virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> -                                         _("unknown clock adjustment '%s'"), tmp);
> +                if (virStrToLong_ll(tmp, NULL, 10,
> +                                    &def->clock.data.variable.adjustment) < 0) {
> +                    virDomainReportError(VIR_ERR_XML_ERROR,
> +                                         _("unknown clock adjustment '%s'"),
> +                                         tmp);
>                      goto error;
>                  }
>                  switch (def->clock.offset) {
> @@ -8103,7 +8108,6 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>                      break;
>                  }
>                  def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_VARIABLE;
> -                def->clock.data.variable.adjustment = val;
>              }
>              VIR_FREE(tmp);
>          } else {
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 2330fa1..7579327 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -570,14 +570,15 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
>      if (!mode) {
>          perms->mode = defaultmode;
>      } else {
> -        char *end = NULL;
> -        perms->mode = strtol(mode, &end, 8);
> -        if (*end || (perms->mode & ~0777)) {
> +        int tmp;
> +
> +        if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) {
>              VIR_FREE(mode);
>              virStorageReportError(VIR_ERR_XML_ERROR,
>                                    "%s", _("malformed octal mode"));
>              goto error;
>          }
> +        perms->mode = tmp;


I'm curious why in the case of clock.data.variable.adjustment, you
switched it to do the conversion directly into the object attribute,
while in this case you switched it in the opposite direction.


ACK, in any case.