[libvirt] [PATCH v3 3/5] Add a new function doCoreDump

[Eric Blake]

On 11/30/2010 12:13 AM, Hu Tao wrote:
> This patch prepares for the next patch.
> ---
>  src/qemu/qemu_driver.c |  145 ++++++++++++++++++++++++++----------------------
>  1 files changed, 78 insertions(+), 67 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ad67e52..80ce9f6 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -138,6 +138,7 @@ struct _qemuDomainObjPrivate {
>  };
>  
>  static int getCompressionType(struct qemud_driver *driver);
> +static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path, int compress);
>  
>  static int qemudShutdown(void);
>  
> @@ -6057,6 +6058,81 @@ cleanup:
>      return ret;
>  }
>  
> +static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path, int compress)
> +{
> +    int fd = -1;
> +    int ret = -1;
> +    qemuDomainObjPrivatePtr priv;
> +
> +    priv = vm->privateData;

You failed to move the qemuDomainObjBeginJobWithDriver() check to this
factored function, but deleted it from the caller - that's a surefire
way to crash the program, especially since other code remaining in in
qemudDomainCoreDump also wants to use qemuDomainObjEnterMonitorWithDriver.

> +
> +    if (!virDomainObjIsActive(vm)) {
> +        qemuReportError(VIR_ERR_OPERATION_INVALID,
> +                        "%s", _("domain is not running"));
> +        goto endjob;
> +    }

Therefore, it's easier to leave this check in the caller, and assume
that doCoreDump will only be called by someone that already owns the
locks and has verified that vm is still running (but it does mean that
your new caller in 4/5 will have to comply with those assumptions).

> +
> +    /* Create an empty file with appropriate ownership.  */
> +    if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) {
> +        qemuReportError(VIR_ERR_OPERATION_FAILED,
> +                        _("failed to create '%s'"), path);
> +        goto endjob;
> +    }

At which point 'endjob' is not the best name for the label within
doCoreDump; sticking with 'cleanup' seems better.

> @@ -6109,35 +6182,6 @@ static int qemudDomainCoreDump(virDomainPtr dom,
>      }
>      priv = vm->privateData;
>  
> -    if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> -        goto cleanup;
> -
> -    if (!virDomainObjIsActive(vm)) {
> -        qemuReportError(VIR_ERR_OPERATION_INVALID,
> -                        "%s", _("domain is not running"));
> -        goto endjob;
> -    }

In other words, this portion should probably not be moved out.

I do like the idea of this factorization though, so I'm looking forward
to v4.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101130/a215ba87/attachment-0001.sig>