Re: [libvirt] [PATCH REBASE 3/5] utils: export virCopyError

On 04.05.2018 16:58, John Ferlan wrote: > > > On 05/03/2018 03:39 AM, Nikolay Shirokovskiy wrote: >> >> >> On 01.05.2018 01:03, John Ferlan wrote: >>> >>> >>> On 04/18/2018 10:44 AM, Nikolay Shirokovskiy wrote: >>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; >>>> --- >>>> src/libvirt_private.syms | 1 + >>>> src/util/virerror.c | 12 +++++++++--- >>>> src/util/virerror.h | 1 + >>>> 3 files changed, 11 insertions(+), 3 deletions(-) >>>> >>> >>> NACK, you should be using virErrorCopyNew >>> >>> John >> >> I introduced monError in qemuDomainObjPrivatePtr in next patch and it is not a pointer so >> I need this function. I did not make monError pointer for the same reason it is not pointer >> in monitor object - I use monError both as flag and as error message. >> > > I saw what you did - the fact is virCopyError is coded as private to the > module. Just making it global because you have a use for it is I believe > incorrect. But why?

> > Ironically in the next patch you have: > > + virErrorPtr err = qemuMonitorLastError(mon); > + > + virCopyError(err, &priv->monError); > > > The first call isn't guaranteed to set @err and all you're essentially > doing is wanting to copy from mon->lastError to priv->monError or > perhaps similar to virCopyLastError. It is ok that error can turn from non-NULL to NULL on copy due to OOM conditions or whaever. It is just as in previous patch. We use priv->monError for 2 purpuses. And thus qemuProcessNotifyMonitorError sets priv->monError.code explicitly if it still set to VIR_ERR_OK after copy.

> > Maybe some new API in virerror.c should handle that. > Not sure we need it at this point. But may be I miss something, please share your vision in more detail then.