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@virtuozzo.com> --- 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