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

Re: [libvirt] [PATCH v4] qemu: Add callback struct for qemuBuildCommandLine



On Fri, May 17, 2013 at 06:34:24PM +0800, Osier Yang wrote:
> Since 0d70656afded, it starts to access the sysfs files to build
> the qemu command line (by virSCSIDeviceGetSgName, which is to find
> out the scsi generic device name by adpater:bus:target:unit), there
> is no way to work around, qemu wants to see the scsi generic device
> like "/dev/sg6" anyway.
> 
> And there might be other places which need to access sysfs files
> when building qemu command line in future.
> 
> Instead of increasing the arguments of qemuBuildCommandLine, this
> introduces a new callback for qemuBuildCommandLine, and thus tests
> can register their own callbacks for sysfs test input files accessing.
> 
> * src/qemu/qemu_command.h: (New callback struct
>                             qemuBuildCommandLineCallbacks;
>                             extern buildCommandLineCallbacks)
> * src/qemu/qemu_command.c: (wire up the callback struct)
> * src/qemu/qemu_driver.c: (Use the new syntax of qemuBuildCommandLine)
> * src/qemu/qemu_hotplug.c: Likewise
> * src/qemu/qemu_process.c: Likewise
> * tests/qemuxml2argvtest.c: (Helper testSCSIDeviceGetSgName;
>                              callback struct testCallbacks;
>                              Use the callback struct)
> * src/tests/qemuxmlnstest.c: (Like above)
> ---
>  src/qemu/qemu_command.c  | 22 ++++++++++++++--------
>  src/qemu/qemu_command.h  | 19 ++++++++++++++++---
>  src/qemu/qemu_driver.c   |  3 ++-
>  src/qemu/qemu_hotplug.c  |  6 ++++--
>  src/qemu/qemu_process.c  |  3 ++-
>  tests/qemuxml2argvtest.c | 21 ++++++++++++++++++++-
>  tests/qemuxmlnstest.c    | 21 ++++++++++++++++++++-
>  7 files changed, 78 insertions(+), 17 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c268a3a..33a1910 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4757,17 +4757,18 @@ qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev)
>  
>  char *
>  qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
> -                           virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED)
> +                           virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED,
> +                           qemuBuildCommandLineCallbacksPtr callbacks)
>  {
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      char *sg = NULL;
>  
> -    if (!(sg = virSCSIDeviceGetSgName(dev->source.subsys.u.scsi.adapter,
> -                                      dev->source.subsys.u.scsi.bus,
> -                                      dev->source.subsys.u.scsi.target,
> -                                      dev->source.subsys.u.scsi.unit))) {
> +    sg = (callbacks->qemuGetSCSIDeviceSgName)(dev->source.subsys.u.scsi.adapter,
> +                                              dev->source.subsys.u.scsi.bus,
> +                                              dev->source.subsys.u.scsi.target,
> +                                              dev->source.subsys.u.scsi.unit);
> +    if (!sg)
>          goto error;
> -    }
>  
>      virBufferAsprintf(&buf, "file=/dev/%s,if=none", sg);
>      virBufferAsprintf(&buf, ",id=%s-%s",
> @@ -6405,6 +6406,10 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
>      return 0;
>  }
>  
> +qemuBuildCommandLineCallbacks buildCommandLineCallbacks = {
> +    .qemuGetSCSIDeviceSgName = virSCSIDeviceGetSgName,
> +};
> +
>  /*
>   * Constructs a argv suitable for launching qemu with config defined
>   * for a given virtual machine.
> @@ -6422,7 +6427,8 @@ qemuBuildCommandLine(virConnectPtr conn,
>                       const char *migrateFrom,
>                       int migrateFd,
>                       virDomainSnapshotObjPtr snapshot,
> -                     enum virNetDevVPortProfileOp vmop)
> +                     enum virNetDevVPortProfileOp vmop,
> +                     qemuBuildCommandLineCallbacksPtr callbacks)
>  {
>      virErrorPtr originalError = NULL;
>      int i, j;
> @@ -8254,7 +8260,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>                  char *drvstr;
>  
>                  virCommandAddArg(cmd, "-drive");
> -                if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps)))
> +                if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps, callbacks)))
>                      goto error;
>                  virCommandAddArg(cmd, drvstr);
>                  VIR_FREE(drvstr);
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 36dfa6b..133e0b2 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -51,6 +51,16 @@
>  # define QEMU_WEBSOCKET_PORT_MIN  5700
>  # define QEMU_WEBSOCKET_PORT_MAX  65535
>  
> +typedef struct _qemuBuildCommandLineCallbacks qemuBuildCommandLineCallbacks;
> +typedef qemuBuildCommandLineCallbacks *qemuBuildCommandLineCallbacksPtr;
> +struct _qemuBuildCommandLineCallbacks {
> +    char * (*qemuGetSCSIDeviceSgName) (const char *adapter,
> +                                       unsigned int bus,
> +                                       unsigned int target,
> +                                       unsigned int unit);
> +};
> +
> +extern qemuBuildCommandLineCallbacks buildCommandLineCallbacks;
>  
>  virCommandPtr qemuBuildCommandLine(virConnectPtr conn,
>                                     virQEMUDriverPtr driver,
> @@ -61,8 +71,9 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn,
>                                     const char *migrateFrom,
>                                     int migrateFd,
>                                     virDomainSnapshotObjPtr current_snapshot,
> -                                   enum virNetDevVPortProfileOp vmop)
> -    ATTRIBUTE_NONNULL(1);
> +                                   enum virNetDevVPortProfileOp vmop,
> +                                   qemuBuildCommandLineCallbacksPtr callbacks)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(11);
>  
>  /* Generate string for arch-specific '-device' parameter */
>  char *
> @@ -142,7 +153,9 @@ char * qemuBuildUSBHostdevDevStr(virDomainHostdevDefPtr dev,
>                                   virQEMUCapsPtr qemuCaps);
>  
>  char * qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
> -                                  virQEMUCapsPtr qemuCaps);
> +                                  virQEMUCapsPtr qemuCaps,
> +                                  qemuBuildCommandLineCallbacksPtr callbacks)
> +    ATTRIBUTE_NONNULL(3);
>  char * qemuBuildSCSIHostdevDevStr(virDomainDefPtr def,
>                                    virDomainHostdevDefPtr dev,
>                                    virQEMUCapsPtr qemuCaps);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 203cc6d..0665131 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5362,7 +5362,8 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn,
>  
>      if (!(cmd = qemuBuildCommandLine(conn, driver, def,
>                                       &monConfig, monitor_json, qemuCaps,
> -                                     NULL, -1, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP)))
> +                                     NULL, -1, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
> +                                     &buildCommandLineCallbacks)))
>          goto cleanup;
>  
>      ret = virCommandToString(cmd);
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index d037c9d..77d9f4f 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1226,7 +1226,8 @@ qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver,
>      if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, 0) < 0)
>          goto cleanup;
>  
> -    if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps)))
> +    if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps,
> +                                              &buildCommandLineCallbacks)))
>          goto cleanup;
>  
>      if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, priv->qemuCaps)))
> @@ -2543,7 +2544,8 @@ qemuDomainDetachHostScsiDevice(virQEMUDriverPtr driver,
>          return -1;
>      }
>  
> -    if (!(drvstr = qemuBuildSCSIHostdevDrvStr(detach, priv->qemuCaps)))
> +    if (!(drvstr = qemuBuildSCSIHostdevDrvStr(detach, priv->qemuCaps,
> +                                              &buildCommandLineCallbacks)))
>          goto cleanup;
>      if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, detach, priv->qemuCaps)))
>          goto cleanup;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 4a7c612..95e712c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3605,7 +3605,8 @@ int qemuProcessStart(virConnectPtr conn,
>      VIR_DEBUG("Building emulator command line");
>      if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig,
>                                       priv->monJSON, priv->qemuCaps,
> -                                     migrateFrom, stdin_fd, snapshot, vmop)))
> +                                     migrateFrom, stdin_fd, snapshot, vmop,
> +                                     &buildCommandLineCallbacks)))
>          goto cleanup;
>  
>      /* now that we know it is about to start call the hook if present */
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index d853308..e103925 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -82,6 +82,24 @@ typedef enum {
>      FLAG_JSON               = 1 << 3,
>  } virQemuXML2ArgvTestFlags;
>  
> +static char *
> +testSCSIDeviceGetSgName(const char *adapter ATTRIBUTE_UNUSED,
> +                        unsigned int bus ATTRIBUTE_UNUSED,
> +                        unsigned int target ATTRIBUTE_UNUSED,
> +                        unsigned int unit ATTRIBUTE_UNUSED)
> +{
> +    char *sg = NULL;
> +
> +    if (VIR_STRDUP(sg, "sg0") < 0)
> +        return NULL;
> +
> +    return sg;
> +}
> +
> +static qemuBuildCommandLineCallbacks testCallbacks = {
> +    .qemuGetSCSIDeviceSgName = testSCSIDeviceGetSgName,
> +};

I think I suggested putting this 'qemuBuildCommandLineCallbacks' instance
in testutilsqemu.{c,h} so that it could be share in all test cases, instead
of them having to duplicate it as you've done here:


> diff --git a/tests/qemuxmlnstest.c b/tests/qemuxmlnstest.c
> index 952b8e2..961405b 100644
> --- a/tests/qemuxmlnstest.c
> +++ b/tests/qemuxmlnstest.c
> @@ -26,6 +26,24 @@
>  static const char *abs_top_srcdir;
>  static virQEMUDriver driver;
>  
> +static char *
> +testSCSIDeviceGetSgName(const char *adapter ATTRIBUTE_UNUSED,
> +                        unsigned int bus ATTRIBUTE_UNUSED,
> +                        unsigned int target ATTRIBUTE_UNUSED,
> +                        unsigned int unit ATTRIBUTE_UNUSED)
> +{
> +    char *sg = NULL;
> +
> +    if (VIR_STRDUP(sg, "dummy") < 0)
> +        return NULL;
> +
> +    return sg;
> +}
> +
> +static qemuBuildCommandLineCallbacks testCallbacks = {
> +    .qemuGetSCSIDeviceSgName = testSCSIDeviceGetSgName,
> +};
> +

ACK either way

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]