Re: [libvirt] [libvirt PATCH v7 4/6] xen_common: Change xenParsePCIList to using virConfGetValueStringList

On 09/20/2018 09:28 AM, Fabiano Fidêncio wrote: > The `if(!list || list->type != VIR_CONF_LIST)` check couldn't be > written in a 100% similar way. Instead, we're just checking whether > `virConfGetValueStringList() <= 0` and creating a new function to: > - return -1 in case virConfGetValueStringList fails either due to some > allocation failure or when traversing the list; > - resetting the last error and return 0 otherwise; > > Taking this approach we can have the behaviour with the new code as > close as possible to the old one. > > Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt; > --- > src/xenconfig/xen_common.c | 34 ++++++++++++++++++++++++++-------- > 1 file changed, 26 insertions(+), 8 deletions(-) > > diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c > index 7b3e5c3b44..9133998cd7 100644 > --- a/src/xenconfig/xen_common.c > +++ b/src/xenconfig/xen_common.c > @@ -458,22 +458,40 @@ xenParsePCI(char *entry) > return hostdev; > } > Things you will get used to eventually... New functions with 2 blank lines and each argument on one line, although I don't believe you need to pass errorCode here.

> +static int > +xenHandleConfGetValueStringListErrors(int ret, int errorCode) > +{ > + if (ret < 0) { > + /* It means virConfGetValueStringList() 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 (errorCode != VIR_ERR_INTERNAL_ERROR) > + return -1; > + > + /* If we did fall through the switch, then ignore and clear the > + * last error. */ > + virResetLastError(); > + } > + return 0; > +} > > static int > xenParsePCIList(virConfPtr conf, virDomainDefPtr def) > { > - virConfValuePtr list = virConfGetValue(conf, "pci"); > + VIR_AUTOPTR(virString) pcis = NULL; > + virString *entries = NULL; > + int rc; > > - if (!list || list->type != VIR_CONF_LIST) > - return 0; > + if ((rc = virConfGetValueStringList(conf, "pci", false, &pcis)) <= 0) > + return xenHandleConfGetValueStringListErrors(rc, virGetLastErrorCode()); No need to pass virGetLastErrorCode - in the 3 uses I see it's not changing and the called method can make that call. I can alter before pushing if you're fine with that.

Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John [...]