On 05/07/2018 04:33 AM, Nikolay Shirokovskiy wrote: > > > 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@xxxxxxxxxxxxx> >>>>> --- >>>>> 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? > Because virErrorCopyNew is designated to "externally" use virCopyError. >> >> 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. > It's "possible" from the above code that @priv->monError would have nothing filled in based on virCopyError logic, so how is that better than what's being done now? That's why I figured that changing the innards of virCopyLastError would be more beneficial in the long run. >> >> 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. > It's not my patch - I'll review whatever comes next. I've provided suggestions and comments. John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
