[libvirt] [libvirt PATCH v6 13/15] xen_common: Change xenParsePCIList to using virConfGetValueStringList

[John Ferlan]

On 09/18/2018 02:48 PM, Fabiano Fidêncio wrote:
> The `if (!list || list->type != VIR_CONF_LIST)` couldn't be written in a
> 100% similar way as `if (virConfGetValueStringList(conf, "pci", false,
> &pcis) <= 0)` leads us to just ignore any error and return 0 in case of
> failure. However, for what's needed in this function, this is the
> closest to the original that we can get and it shouldn't change the
> function behaviour.
> 
> Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> ---
>  src/xenconfig/xen_common.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 09f93ba449..9ad081e56b 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -462,27 +462,30 @@ xenParsePCI(char *entry)
>  static int
>  xenParsePCIList(virConfPtr conf, virDomainDefPtr def)
>  {
> -    virConfValuePtr list = virConfGetValue(conf, "pci");
> +    char **pcis = NULL, **entries;
> +    int ret = -1;
>  
> -    if (!list || list->type != VIR_CONF_LIST)
> +    if (virConfGetValueStringList(conf, "pci", false, &pcis) <= 0)
>          return 0;

Since this call passes @false, that means virConfGetValueStringList
processing will "fallthrough" the switch for VIR_CONF_STRING and thus
generate an error and return -1. So to that degree OK, I agree this is
no different than the previous code.  I'll point out it's not a great
design, but yes, no worse than before.

Still if < 0, a virReportError was generated, thus we need to
virResetLastError before returning 0 indicating we're going to ignore
whether the "pci" value was found and whether it was the right type
(e.g. since @false we only want VIR_CONF_LIST values.

This will though cause us to miss memory allocation errors, but I have a
feeling we'd hit another one real soon.

So, this will be ugly and makes assumptions that may not be true in the
called function forever, but would look something like this:

    int rc;

    if ((rc = virConfGetValueStringList(...)) <= 0) {
        if (rc < 0) {
            /* IOW: If called function didn't fail because the
             * cval->type switch fell through - since we're passing
             * @compatString == false - assumes failures for memory
             * allocation and VIR_CONF_LIST traversal failure
             * should cause -1 to be returned to the caller with
             * the error message set. */
            if (virGetLastErrorCode() != VIR_ERR_INTERNAL_ERROR)
                return -1;

            /* If we did fall through the switch, then ignore and
             * clear the last error./
            virResetLastError();
        }

        return 0;
    }

See, really ugly and not 100% supportable, but it should cover the
existing cases.

John

>  
> -    for (list = list->list; list; list = list->next) {
> +    for (entries = pcis; *entries; entries++) {
> +        char *entry = *entries;
>          virDomainHostdevDefPtr hostdev;
>  
> -        if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
> -            continue;
> -
> -        if (!(hostdev = xenParsePCI(list->str)))
> -            return -1;
> +        if (!(hostdev = xenParsePCI(entry)))
> +            goto cleanup;
>  
>          if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) {
>              virDomainHostdevDefFree(hostdev);
> -            return -1;
> +            goto cleanup;
>          }
>      }
>  
> -    return 0;
> +    ret = 0;
> +
> + cleanup:
> +    virStringListFree(pcis);
> +    return ret;
>  }
>  
>  
>