On Fri, May 04, 2018 at 04:46:47AM +0100, ramyelkest wrote: > Many places in the code call virGetLastError() just to check the > raised error code, or domain. However virGetLastError() can return > NULL, so the code has to check for that as well. s/as well/first. > > So Instead we create functions virGetLastErrorCode and virGetLastErrorDomain > (in addition to the existing virGetLastErrorMessage) that always return a > valid error code/domain/message, to simplify callers. No paragraph, how about "This patch therefore introduces virGetLasErrorCode function which always returns a valid error code, thus dropping the need to perform any checks on the error object". > > Also created virHasLastErrorCode for completion > > My first commit, for: > https://wiki.libvirt.org/page/BiteSizedTasks#Add_and_use_virGetLastErrorCode.28.29 > You mentioned this in the cover letter already, no need to have it in the commit messages too ;). > Signed-off-by: Ramy Elkest <ramyelkest@xxxxxxxxx> > --- > include/libvirt/virterror.h | 3 +++ > src/libvirt_public.syms | 7 +++++ > src/util/virerror.c | 63 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 73 insertions(+) > > diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h > index 3e7c7a0..8336258 100644 > --- a/include/libvirt/virterror.h > +++ b/include/libvirt/virterror.h > @@ -344,6 +344,9 @@ void virResetLastError (void); > void virResetError (virErrorPtr err); > void virFreeError (virErrorPtr err); > > +int virHasLastError (void); > +int virGetLastErrorCode (void); > +int virGetLastErrorDomain (void); > const char * virGetLastErrorMessage (void); > > virErrorPtr virConnGetLastError (virConnectPtr conn); > diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms > index 95df3a0..3a641a3 100644 > --- a/src/libvirt_public.syms > +++ b/src/libvirt_public.syms > @@ -785,4 +785,11 @@ LIBVIRT_4.1.0 { > virStoragePoolLookupByTargetPath; > } LIBVIRT_3.9.0; > > +LIBVIRT_4.4.0 { > + global: > + virHasLastError; > + virGetLastErrorCode; > + virGetLastErrorDomain; > +} LIBVIRT_4.1.0; > + > # .... define new API here using predicted next version number .... > diff --git a/src/util/virerror.c b/src/util/virerror.c > index c000b00..818af2e 100644 > --- a/src/util/virerror.c > +++ b/src/util/virerror.c > @@ -272,6 +272,69 @@ virGetLastError(void) > > > /** > + * virHasLastError: > + * > + * Check for a reported last error > + * > + * The error object is kept in thread local storage, so separate > + * threads can safely access this concurrently. > + * > + * Returns 1 if there is an error and the error code is not VIR_ERR_OK, > + otherwise returns 0 > + */ > +int > +virHasLastError(void) > +{ > + virErrorPtr err = virLastErrorObject(); > + if (!err || err->code == VIR_ERR_OK) > + return 0; > + return 1; > +} As you noted in the cover letter, ^this one is essentially identical to virGetLastErrorCode, minus the "1 vs err->code" and since VIR_ERR_OK == 0, you can use virGetLastErrorCode the same way as you used virHasLastError as a replacement in many occurrences of virGetLastError(), so I don't see the need for this function. > + > + > +/** > + * virGetLastErrorCode: > + * > + * Get the most recent error code > + * > + * The error object is kept in thread local storage, so separate > + * threads can safely access this concurrently. > + * > + * Returns the most recent error code in this thread, > + * or VIR_ERR_OK if none is set > + */ > +int > +virGetLastErrorCode(void) > +{ > + virErrorPtr err = virLastErrorObject(); > + if (!err) > + return VIR_ERR_OK; > + return err->code; > +} > + > + > +/** > + * virGetLastErrorDomain: > + * > + * Get the most recent error domain > + * > + * The error object is kept in thread local storage, so separate > + * threads can safely access this concurrently. > + * > + * Returns the most recent error code in this thread, > + * or VIR_FROM_NONE if none is set > + */ > +int > +virGetLastErrorDomain(void) > +{ > + virErrorPtr err = virLastErrorObject(); > + if (!err) > + return VIR_FROM_NONE; > + return err->domain; > +} I can see a need for ^this function, even though we don't have an immediate use case for it internally, it's still a public API which someone else might consume, otherwise they'd have to query the object itself. Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
