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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list