[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] qemuProcessStart: Switch to flags instead of bunch booleans



On Mon, Apr 16, 2012 at 05:09:59PM +0200, Michal Privoznik wrote:
> Currently, we have 3 boolean arguments we have to pass
> to qemuProcessStart(). As libvirt grows it is harder and harder
> to remember them and their position. Therefore we should
> switch to flags instead.
> ---
> diff to v1:
> -fix a test for START_PAUSED flag
> 
>  src/qemu/qemu_driver.c    |   45 +++++++++++++++++++++++++++++----------------
>  src/qemu/qemu_migration.c |    7 ++++---
>  src/qemu/qemu_process.c   |   21 +++++++++++++--------
>  src/qemu/qemu_process.h   |   12 ++++++++----
>  4 files changed, 54 insertions(+), 31 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 65ed290..436ef37 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1351,10 +1351,16 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
>      virDomainPtr dom = NULL;
>      virDomainEventPtr event = NULL;
>      virDomainEventPtr event2 = NULL;
> +    unsigned int start_flags = VIR_QEMU_PROCESS_START_COLD;
>  
>      virCheckFlags(VIR_DOMAIN_START_PAUSED |
>                    VIR_DOMAIN_START_AUTODESTROY, NULL);
>  
> +    if (flags & VIR_DOMAIN_START_PAUSED)
> +        start_flags |= VIR_QEMU_PROCESS_START_PAUSED;
> +    if (flags & VIR_DOMAIN_START_AUTODESTROY)
> +        start_flags |= VIR_QEMU_PROCESS_START_AUTODESROY;
> +
>      qemuDriverLock(driver);
>      if (!(def = virDomainDefParseString(driver->caps, xml,
>                                          QEMU_EXPECTED_VIRT_TYPES,
> @@ -1383,10 +1389,9 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
>      if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
>          goto cleanup; /* XXXX free the 'vm' we created ? */
>  
> -    if (qemuProcessStart(conn, driver, vm, NULL, true,
> -                         (flags & VIR_DOMAIN_START_PAUSED) != 0,
> -                         (flags & VIR_DOMAIN_START_AUTODESTROY) != 0,
> -                         -1, NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_CREATE) < 0) {
> +    if (qemuProcessStart(conn, driver, vm, NULL, -1, NULL, NULL,
> +                         VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
> +                         start_flags) < 0) {
>          virDomainAuditStart(vm, "booted", false);
>          if (qemuDomainObjEndJob(driver, vm) > 0)
>              qemuDomainRemoveInactive(driver, vm);
> @@ -4138,9 +4143,9 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
>      }
>  
>      /* Set the migration source and start it up. */
> -    ret = qemuProcessStart(conn, driver, vm, "stdio", false, true,
> -                           false, *fd, path, NULL,
> -                           VIR_NETDEV_VPORT_PROFILE_OP_RESTORE);
> +    ret = qemuProcessStart(conn, driver, vm, "stdio", *fd, path, NULL,
> +                           VIR_NETDEV_VPORT_PROFILE_OP_RESTORE,
> +                           VIR_QEMU_PROCESS_START_PAUSED);
>  
>      if (intermediatefd != -1) {
>          if (ret < 0) {
> @@ -4710,6 +4715,10 @@ qemuDomainObjStart(virConnectPtr conn,
>      bool autodestroy = (flags & VIR_DOMAIN_START_AUTODESTROY) != 0;
>      bool bypass_cache = (flags & VIR_DOMAIN_START_BYPASS_CACHE) != 0;
>      bool force_boot = (flags & VIR_DOMAIN_START_FORCE_BOOT) != 0;
> +    unsigned int start_flags = VIR_QEMU_PROCESS_START_COLD;
> +
> +    start_flags |= start_paused ? VIR_QEMU_PROCESS_START_PAUSED : 0;
> +    start_flags |= autodestroy ? VIR_QEMU_PROCESS_START_AUTODESROY : 0;
>  
>      /*
>       * If there is a managed saved state restore it instead of starting
> @@ -4741,9 +4750,8 @@ qemuDomainObjStart(virConnectPtr conn,
>          }
>      }
>  
> -    ret = qemuProcessStart(conn, driver, vm, NULL, true, start_paused,
> -                           autodestroy, -1, NULL, NULL,
> -                           VIR_NETDEV_VPORT_PROFILE_OP_CREATE);
> +    ret = qemuProcessStart(conn, driver, vm, NULL, -1, NULL, NULL,
> +                           VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags);
>      virDomainAuditStart(vm, "booted", ret >= 0);
>      if (ret >= 0) {
>          virDomainEventPtr event =
> @@ -11027,9 +11035,10 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>              if (config)
>                  virDomainObjAssignDef(vm, config, false);
>  
> -            rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL,
> -                                  false, true, false, -1, NULL, snap,
> -                                  VIR_NETDEV_VPORT_PROFILE_OP_CREATE);
> +            rc = qemuProcessStart(snapshot->domain->conn,
> +                                  driver, vm, NULL, -1, NULL, snap,
> +                                  VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
> +                                  VIR_QEMU_PROCESS_START_PAUSED);
>              virDomainAuditStart(vm, "from-snapshot", rc >= 0);
>              detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT;
>              event = virDomainEventNewFromObj(vm,
> @@ -11114,12 +11123,16 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>                       VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
>              /* Flush first event, now do transition 2 or 3 */
>              bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0;
> +            unsigned int start_flags = 0;
> +
> +            start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0;
>  
>              if (event)
>                  qemuDomainEventQueue(driver, event);
> -            rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL,
> -                                  false, paused, false, -1, NULL, NULL,
> -                                  VIR_NETDEV_VPORT_PROFILE_OP_CREATE);
> +            rc = qemuProcessStart(snapshot->domain->conn,
> +                                  driver, vm, NULL, -1, NULL, NULL,
> +                                  VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
> +                                  start_flags);
>              virDomainAuditStart(vm, "from-snapshot", rc >= 0);
>              if (rc < 0) {
>                  if (!vm->persistent) {
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index dc4d616..fc83805 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1303,9 +1303,10 @@ qemuMigrationPrepareAny(struct qemud_driver *driver,
>      /* Start the QEMU daemon, with the same command-line arguments plus
>       * -incoming $migrateFrom
>       */
> -    if (qemuProcessStart(dconn, driver, vm, migrateFrom, false, true,
> -                         true, dataFD[0], NULL, NULL,
> -                         VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) < 0) {
> +    if (qemuProcessStart(dconn, driver, vm, migrateFrom, dataFD[0], NULL, NULL,
> +                         VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START,
> +                         VIR_QEMU_PROCESS_START_PAUSED |
> +                         VIR_QEMU_PROCESS_START_AUTODESROY) < 0) {
>          virDomainAuditStart(vm, "migrated", false);
>          /* Note that we don't set an error here because qemuProcessStart
>           * should have already done that.
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 692fc32..ecbf039 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3271,13 +3271,11 @@ int qemuProcessStart(virConnectPtr conn,
>                       struct qemud_driver *driver,
>                       virDomainObjPtr vm,
>                       const char *migrateFrom,
> -                     bool cold_boot,
> -                     bool start_paused,
> -                     bool autodestroy,
>                       int stdin_fd,
>                       const char *stdin_path,
>                       virDomainSnapshotObjPtr snapshot,
> -                     enum virNetDevVPortProfileOp vmop)
> +                     enum virNetDevVPortProfileOp vmop,
> +                     unsigned int flags)
>  {
>      int ret;
>      off_t pos = -1;
> @@ -3290,6 +3288,12 @@ int qemuProcessStart(virConnectPtr conn,
>      unsigned long cur_balloon;
>      int i;
>  
> +    /* Okay, these are just internal flags,
> +     * but doesn't hurt to check */
> +    virCheckFlags(VIR_QEMU_PROCESS_START_COLD |
> +                  VIR_QEMU_PROCESS_START_PAUSED |
> +                  VIR_QEMU_PROCESS_START_AUTODESROY, -1);
> +
>      hookData.conn = conn;
>      hookData.vm = vm;
>      hookData.driver = driver;
> @@ -3434,7 +3438,8 @@ int qemuProcessStart(virConnectPtr conn,
>          goto cleanup;
>  
>      VIR_DEBUG("Checking for CDROM and floppy presence");
> -    if (qemuDomainCheckDiskPresence(driver, vm, cold_boot) < 0)
> +    if (qemuDomainCheckDiskPresence(driver, vm,
> +                                    flags & VIR_QEMU_PROCESS_START_COLD) < 0)
>          goto cleanup;
>  
>      VIR_DEBUG("Setting up domain cgroup (if required)");
> @@ -3633,7 +3638,7 @@ int qemuProcessStart(virConnectPtr conn,
>      VIR_DEBUG("Handshake complete, child running");
>  
>      if (migrateFrom)
> -        start_paused = true;
> +        flags |= VIR_QEMU_PROCESS_START_PAUSED;
>  
>      if (ret == -1) /* The VM failed to start; tear filters before taps */
>          virDomainConfVMNWFilterTeardown(vm);
> @@ -3708,7 +3713,7 @@ int qemuProcessStart(virConnectPtr conn,
>      }
>      qemuDomainObjExitMonitorWithDriver(driver, vm);
>  
> -    if (!start_paused) {
> +    if (!(flags & VIR_QEMU_PROCESS_START_PAUSED)) {
>          VIR_DEBUG("Starting domain CPUs");
>          /* Allow the CPUS to start executing */
>          if (qemuProcessStartCPUs(driver, vm, conn,
> @@ -3726,7 +3731,7 @@ int qemuProcessStart(virConnectPtr conn,
>                               VIR_DOMAIN_PAUSED_USER);
>      }
>  
> -    if (autodestroy &&
> +    if (flags & VIR_QEMU_PROCESS_START_AUTODESROY &&
>          qemuProcessAutoDestroyAdd(driver, vm, conn) < 0)
>          goto cleanup;
>  
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 5cb5ffc..2d75b005 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -44,17 +44,21 @@ void qemuProcessReconnectAll(virConnectPtr conn, struct qemud_driver *driver);
>  
>  int qemuProcessAssignPCIAddresses(virDomainDefPtr def);
>  
> +typedef enum {
> +    VIR_QEMU_PROCESS_START_COLD         = 1 << 0,
> +    VIR_QEMU_PROCESS_START_PAUSED       = 1 << 1,
> +    VIR_QEMU_PROCESS_START_AUTODESROY   = 1 << 2,
> +} qemuProcessStartFlags;
> +
>  int qemuProcessStart(virConnectPtr conn,
>                       struct qemud_driver *driver,
>                       virDomainObjPtr vm,
>                       const char *migrateFrom,
> -                     bool cold_boot,
> -                     bool start_paused,
> -                     bool autodestroy,
>                       int stdin_fd,
>                       const char *stdin_path,
>                       virDomainSnapshotObjPtr snapshot,
> -                     enum virNetDevVPortProfileOp vmop);
> +                     enum virNetDevVPortProfileOp vmop,
> +                     unsigned int flags);
>  
>  void qemuProcessStop(struct qemud_driver *driver,
>                       virDomainObjPtr vm,


ACK, that's better now


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]