On Thu, Sep 20, 2018 at 10:18 PM, John Ferlan <jfer...@redhat.com> wrote:
> > > 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 <fiden...@redhat.com> > > --- > > 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. > Okay, noted! > > > +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. > Yes, I'm fine with that. Please, do the changes before pushing! :-) > > Reviewed-by: John Ferlan <jfer...@redhat.com> > > John > > > [...] >
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list