[libvirt] [PATCH] qemu_domain: add timestamp in tainting of guests log

[John Ferlan]

On 01/16/2017 04:19 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao at gmail.com>
> 
> We lacked of timestamp in tainting of guests log,
> which bring troubles for finding guest issues:
> such as whether a guest powerdown caused by qemu-monitor-command
> or others issues inside guests.
> If we had timestamp in tainting of guests log,
> it would be helpful when checking guest's logs(eg. /var/log/messages).
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao at gmail.com>
> ---
>  src/qemu/qemu_domain.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 35baffb..21c90a9 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4001,6 +4001,7 @@ void qemuDomainObjTaint(virQEMUDriverPtr driver,
>      bool closeLog = false;
>  
>      if (virDomainObjTaint(obj, taint)) {
> +        char *timestamp = NULL;
>          char uuidstr[VIR_UUID_STRING_BUFLEN];
>          virUUIDFormat(obj->def->uuid, uuidstr);
>  
> @@ -4018,27 +4019,31 @@ void qemuDomainObjTaint(virQEMUDriverPtr driver,

I think rather than the current flow:

1. Move timestamp decl to the top (because #2 is moving cleanup label)

2. Move cleanup to the bottom of the function (to be consistent w/ other
functions)

3. Move the timestamp fetch to before the LogContextNew

If you really wanted to be creative... Create a patch 1 which reverses
the virDomainObjTaint logic to be:

    if (!virDomainObjTaint(obj, taint))
        return;

Then move the subsequent lines 4 spaces to the left (e.g. reduce
indentation).


>              logCtxt = qemuDomainLogContextNew(driver, obj,
>                                                QEMU_DOMAIN_LOG_CONTEXT_MODE_ATTACH);
>              if (!logCtxt) {
> -                if (orig_err) {
> -                    virSetError(orig_err);
> -                    virFreeError(orig_err);
> -                }
>                  VIR_WARN("Unable to open domainlog");
> -                return;
> +                goto cleanup;
>              }
>              closeLog = true;
>          }
>  
> +        if ((timestamp = virTimeStringNow()) == NULL)
> +            goto cleanup;
> +
>          if (qemuDomainLogContextWrite(logCtxt,
> -                                      "Domain id=%d is tainted: %s\n",
> +                                      "%s: Domain id=%d is tainted: %s\n",
> +                                      timestamp,
>                                        obj->def->id,
>                                        virDomainTaintTypeToString(taint)) < 0)
>              virResetLastError();
> +
> +     cleanup:
>          if (closeLog)
>              qemuDomainLogContextFree(logCtxt);
>          if (orig_err) {
>              virSetError(orig_err);
>              virFreeError(orig_err);
>          }
> +
> +        VIR_FREE(timestamp);

Move this before the "closeLog" check (and the whole cleanup hunk moves
outside the if statement - just looks strange inside

>      }

The order would be:

 cleanup:
    VIR_FREE(timestamp);
    if (closeLog)
        qemuDomainLogContextFree(logCtxt);
    if (orig_err) {
        virSetError(orig_err);
        virFreeError(orig_err);
    }

John

>  }
>  
>