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);
-- 
2.8.3


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

thanks very much, I will make v2 patch soon.


在 2018/7/23 18:37, Michal Prívozník 写道:
> 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
>
> .
>


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list