<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 20, 2018 at 10:18 PM, John Ferlan <span dir="ltr"><<a href="mailto:jferlan@redhat.com" target="_blank">jferlan@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote:<br>
> The `if(!list || list->type != VIR_CONF_LIST)` check couldn't be<br>
> written in a 100% similar way. Instead, we're just checking whether<br>
> `virConfGetValueStringList() <= 0` and creating a new function to:<br>
> - return -1 in case virConfGetValueStringList fails either due to some<br>
>   allocation failure or when traversing the list;<br>
> - resetting the last error and return 0 otherwise;<br>
> <br>
> Taking this approach we can have the behaviour with the new code as<br>
> close as possible to the old one.<br>
> <br>
> Signed-off-by: Fabiano Fidêncio <<a href="mailto:fidencio@redhat.com">fidencio@redhat.com</a>><br>
> ---<br>
>  src/xenconfig/xen_common.c | 34 ++++++++++++++++++++++++++----<wbr>----<br>
>  1 file changed, 26 insertions(+), 8 deletions(-)<br>
> <br>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c<br>
> index 7b3e5c3b44..9133998cd7 100644<br>
> --- a/src/xenconfig/xen_common.c<br>
> +++ b/src/xenconfig/xen_common.c<br>
> @@ -458,22 +458,40 @@ xenParsePCI(char *entry)<br>
>      return hostdev;<br>
>  }<br>
>  <br>
<br>
</span>Things you will get used to eventually... New functions with 2 blank<br>
lines and each argument on one line, although I don't believe you need<br>
to pass errorCode here.<br></blockquote><div><br></div><div>Okay, noted!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +static int<br>
> +<wbr>xenHandleConfGetValueStringLis<wbr>tErrors(int ret, int errorCode)<br>
> +{<br>
> +    if (ret < 0) {<br>
> +        /* It means virConfGetValueStringList() didn't fail because the<br>
> +         * cval->type switch fell through - since we're passing<br>
> +         * @compatString == false - assumes failures for memory allocation<br>
> +         * and VIR_CONF_LIST traversal failure should cause -1 to be<br>
> +         * returned to the caller with the error message set. */<br>
> +        if (errorCode != VIR_ERR_INTERNAL_ERROR)<br>
> +            return -1;<br>
> +<br>
> +        /* If we did fall through the switch, then ignore and clear the<br>
> +         * last error. */<br>
> +        virResetLastError();<br>
> +    }<br>
> +    return 0;<br>
> +}<br>
>  <br>
>  static int<br>
>  xenParsePCIList(virConfPtr conf, virDomainDefPtr def)<br>
>  {<br>
> -    virConfValuePtr list = virConfGetValue(conf, "pci");<br>
> +    VIR_AUTOPTR(virString) pcis = NULL;<br>
> +    virString *entries = NULL;<br>
> +    int rc;<br>
>  <br>
> -    if (!list || list->type != VIR_CONF_LIST)<br>
> -        return 0;<br>
> +    if ((rc = virConfGetValueStringList(<wbr>conf, "pci", false, &pcis)) <= 0)<br>
> +        return xenHandleConfGetValueStringLis<wbr>tErrors(rc, virGetLastErrorCode());<br>
<br>
</div></div>No need to pass virGetLastErrorCode - in the 3 uses I see it's not<br>
changing and the called method can make that call.<br>
<br>
I can alter before pushing if you're fine with that.<br></blockquote><div><br></div><div>Yes, I'm fine with that. Please, do the changes before pushing! :-)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Reviewed-by: John Ferlan <<a href="mailto:jferlan@redhat.com">jferlan@redhat.com</a>><br>
<br>
John<br>
<br>
<br>
[...]<br>
</blockquote></div><br></div></div>