If there is an error getting info from guest agent, then the
control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label
and subsequently continues on 'endagentjob'. Both labels are hit
also in success case, which is why there is a code that tries to
match info obtained from the guest agent with domain definition.
However, if we know that we've reached this area because of an
error (ret = -1) there is no reason for us to attempt finding the
match as the API as whole will end up with an error.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_driver.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2d4b5a8b99..338c609854 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20129,6 +20129,9 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
  endagentjob:
     qemuDomainObjEndAgentJob(vm);
 
+    if (ret < 0)
+        goto cleanup;
+
     if (nfs > 0 || ndisks > 0) {
         if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
             goto cleanup;
-- 
2.26.2

On a Tuesday in 2020, Michal Privoznik wrote:
>If there is an error getting info from guest agent, then the
>control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label
>and subsequently continues on 'endagentjob'. Both labels are hit
>also in success case, which is why there is a code that tries to
>match info obtained from the guest agent with domain definition.

I'm confused by 'exitagent' and 'exitagentjob' being above code
that is only done (or only makes sense) on success. And ret being
set to zero so early - I guess that's due to the nature of the
best-effort information gathering here. But I think it would be
perfectly fine to error out if we fail to get a query job or
the domain dies in the meantime.

Moving the exitagent and endagentjob labels after the cleanup
block would remove the need to check ret.
(i.e. duplicating ExitAgent and EndAgentJob calls - one
pair that would be exectued on success and one pair only on failure)

>However, if we know that we've reached this area because of an
>error (ret = -1) there is no reason for us to attempt finding the
>match as the API as whole will end up with an error.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_driver.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index 2d4b5a8b99..338c609854 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -20129,6 +20129,9 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
>  endagentjob:
>     qemuDomainObjEndAgentJob(vm);
>
>+    if (ret < 0)
>+        goto cleanup;
>+
>     if (nfs > 0 || ndisks > 0) {
>         if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
>             goto cleanup;
>-- 
>2.26.2
>

On 12/1/20 4:39 PM, Ján Tomko wrote:
> On a Tuesday in 2020, Michal Privoznik wrote:
>> If there is an error getting info from guest agent, then the
>> control on qemuDomainGetGuestInfo() jumps onto 'exitagent' label
>> and subsequently continues on 'endagentjob'. Both labels are hit
>> also in success case, which is why there is a code that tries to
>> match info obtained from the guest agent with domain definition.
> 
> I'm confused by 'exitagent' and 'exitagentjob' being above code
> that is only done (or only makes sense) on success. And ret being
> set to zero so early - I guess that's due to the nature of the
> best-effort information gathering here. But I think it would be
> perfectly fine to error out if we fail to get a query job or
> the domain dies in the meantime.
> 
> Moving the exitagent and endagentjob labels after the cleanup
> block would remove the need to check ret.
> (i.e. duplicating ExitAgent and EndAgentJob calls - one
> pair that would be exectued on success and one pair only on failure)

Fair enough. Will post v2.

Michal