[libvirt] [PATCH] Split all QEMU process mangement code into separate file

[Eric Blake]

On 01/28/2011 06:21 AM, Daniel P. Berrange wrote:
> Move the qemudStartVMDaemon and qemudShutdownVMDaemon
> methods into a separate file, renaming them to
> qemuProcessStart, qemuProcessStop. All helper methods
> called by these are also moved & renamed to match
> 
> * src/Makefile.am: Add qemu_process.c/.h
> * src/qemu/qemu_command.c: Add emuDomainAssignPCIAddresses

s/ emu/ qemu/

> * src/qemu/qemu_command.h: Add VNC port min/max
> * src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Add
>   domain event queue helpers
> * src/qemu/qemu_driver.c, src/qemu/qemu_driver.h: Remove
>   all QEMU process startup/shutdown functions
> * src/qemu/qemu_process.c, src/qemu/qemu_process.h: Add
>   all QEMU process startup/shutdown functions

> +++ b/src/qemu/qemu_domain.c
> @@ -29,6 +29,7 @@
>  #include "logging.h"
>  #include "virterror_internal.h"
>  #include "c-ctype.h"
> +#include "event.h"
>  
>  #include <sys/time.h>
>  
> @@ -41,6 +42,61 @@
>  #define timeval_to_ms(tv)       (((tv).tv_sec * 1000ull) + ((tv).tv_usec / 1000))
>  
>  
> +static void qemuDomainEventDispatchFunc(virConnectPtr conn,
> +                                        virDomainEventPtr event,
> +                                        virConnectDomainEventGenericCallback cb,
> +                                        void *cbopaque,
> +                                        void *opaque)
> +{
> +    struct qemud_driver *driver = opaque;
> +
> +    /* Drop the lock whle dispatching, for sake of re-entrancy */

s/whle/while/

> +++ b/src/qemu/qemu_driver.c
> +#  define KVM_CAP_NR_VCPUS 9       /* returns max vcpus per vm */
> +# endif
> +
> +
> +#define timeval_to_ms(tv)       (((tv).tv_sec * 1000ull) + ((tv).tv_usec / 1000))

This will fail 'make syntax-check' if you have cppi installed.

> -static int doStartCPUs(struct qemud_driver *driver, virDomainObjPtr vm, virConnectPtr conn)

Hmm, you also did some reorganization as part of moving functions
between files (this appears early in the old qemu_driver.c, but late in
the new qemu_process.c).  That's okay, it just took me a bit longer to
review.

>  
> +
>  static void
> -qemudAutostartConfigs(struct qemud_driver *driver) {
> +qemuAutostartDomains(struct qemud_driver *driver)

Interesting mix of renames and code movement in the same patch.  I'm not
going to make you split it now, but it might have been nicer as two commits.

>  
> -
> -/**
> - * qemudRemoveDomainStatus
> - *
> - * remove all state files of a domain from statedir
> - *
> - * Returns 0 on success
> - */
>  static int
> -qemudRemoveDomainStatus(struct qemud_driver *driver,
> -                        virDomainObjPtr vm)
> +qemuSecurityInit(struct qemud_driver *driver)
>  {
> -    char ebuf[1024];
> -    char *file = NULL;
> -
> -    if (virAsprintf(&file, "%s/%s.xml", driver->stateDir, vm->def->name) < 0) {
> -        virReportOOMError();
> -        return(-1);
> -    }
> -
> -    if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
> -        VIR_WARN("Failed to remove domain XML for %s: %s",
> -                 vm->def->name, virStrerror(errno, ebuf, sizeof(ebuf)));
> -    VIR_FREE(file);
> +    virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName,
> +                                                      driver->allowDiskFormatProbing);
> +    if (!mgr)
> +        goto error;

Especially in places like this, where the rename interfered with the
diff algorithm to produce a difficult-to-read diff.

> -
> -struct virReconnectDomainData {

It took me a while to see the rename virReconnectDomainData ->
qemuProcessReconnectData.

> -static int qemudStartVMDaemon(virConnectPtr conn,
> -                              struct qemud_driver *driver,
...

> -    if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig,
> -                                     priv->monJSON != 0, qemuCmdFlags,
> -                                     migrateFrom, stdin_fd,
> -                                     vm->current_snapshot, vmop)))
> -        goto cleanup;
> -
> -    if (qemuDomainSnapshotSetCurrentInactive(vm, driver->snapshotDir) < 0)
> -        goto cleanup;

Why was the SetCurrentInactive line commented out in the move?

> +static void
> +qemuProcessReconnect(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque)
> +{

> +    if (virDomainObjUnref(obj) > 0) {
> +        /* We can't get the monitor back, so must kill the VM
> +         * to remove danger of it ending up running twice if
> +         * user tries to start it again later */
> +        qemudShutdownVMDaemon(driver, obj, 0);

Wouldn't this cause a compile error, since you renamed the function to
qemuProcessStop?  Indeed:

cc1: warnings being treated as errors
qemu/qemu_process.c: In function 'qemuProcessReconnect':
qemu/qemu_process.c:1854:9: error: implicit declaration of function
'qemudShutdownVMDaemon' [-Wimplicit-function-declaration]
qemu/qemu_process.c:1854:9: error: nested extern declaration of
'qemudShutdownVMDaemon' [-Wnested-externs]
qemu/qemu_process.c: In function 'qemuProcessStart':
qemu/qemu_process.c:1949:9: error: implicit declaration of function
'fstat' [-Wimplicit-function-declaration]
qemu/qemu_process.c:1949:9: error: nested extern declaration of 'fstat'
[-Wnested-externs]
qemu/qemu_process.c:1954:9: error: implicit declaration of function
'S_ISFIFO' [-Wimplicit-function-declaration]
qemu/qemu_process.c:1954:9: error: nested extern declaration of
'S_ISFIFO' [-Wnested-externs]

Also, you're still missing a clean 'make syntax-check' run; at least
po/POTFILES.in needs work.

I like the idea of this patch, but it needs a bit more work.

-- 
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/20110129/8d210a80/attachment-0001.sig>