Re: [libvirt] [PATCH] libvirt_iohelper: record the libvirt_iohelper's error message at virFileWrapperFdFree

On 07/22/2018 03:40 AM, xinhua.Cao wrote: > Currently iohelper's error log is recorded in virFileWrapperFdClose. > In qemuDomainSaveMemory, it usually fails at qemuMigrationSrcToFile, > and then goto cleanup, so the iohelper error log is not recorded, > and so is the another placement. We now record the error log of > iohelper by move it to the virFileWrapperFdFree record. > There is another problem here, that is, virCommandWait is also not > called, but I can't evaluate this impact. So no changes have been > made here. > --- > src/util/virfile.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/util/virfile.c b/src/util/virfile.c > index 1faeebb..30456ab 100644 > --- a/src/util/virfile.c > +++ b/src/util/virfile.c > @@ -330,9 +330,6 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd) > return 0; > > ret = virCommandWait(wfd->cmd, NULL); > - if (wfd->err_msg && *wfd->err_msg) > - VIR_WARN("iohelper reports: %s", wfd->err_msg); > - > return ret; > } > > @@ -351,6 +348,9 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd) > if (!wfd) > return; > > + if (wfd->err_msg && *wfd->err_msg) > + VIR_WARN("iohelper reports: %s", wfd->err_msg); > + > VIR_FREE(wfd->err_msg); > > virCommandFree(wfd->cmd); > Makes sense. However, your patch lacks Signed-off-By line so I can't push it. It's required according to contributor guidelines: https://libvirt.org/hacking.html And for the virCommandWait() - we should call virCommandAbort() in virFileWrapperFdFree() to make sure no process is left behind. If virCommandWait() is called then virCommandAbort() is a NO-OP. If it isn't, then Abort() will kill iohelper. Michal .