Re: [PATCH 06/15] Forward Mode 'Hostdev' qemu driver implementation

[Laine Stump <laine@xxxxxxxxx>]

On 08/10/2012 12:24 PM, Shradha Shah wrote:

Some explanation is needed in the commit log of what this is being done
here. A cut-paste of the comment in the code would be a good start (any
anyway, that comment can be changed since it's talking about "when there
is a network with <forward mode='hostdev'>", but that "when" is "now" :-)


> Signed-off-by: Shradha Shah <sshah@xxxxxxxxxxxxxx>
> ---
>  src/qemu/qemu_command.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6f6c6cd..bb66364 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -24,6 +24,7 @@
>  #include <config.h>
>  
>  #include "qemu_command.h"
> +#include "qemu_hostdev.h"
>  #include "qemu_capabilities.h"
>  #include "qemu_bridge_filter.h"
>  #include "cpu/cpu.h"
> @@ -5221,12 +5222,38 @@ qemuBuildCommandLine(virConnectPtr conn,
>  
>              actualType = virDomainNetGetActualType(net);
>              if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> +                virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net);
> +                virDomainHostdevDefPtr found;
>                  /* type='hostdev' interfaces are handled in codepath
>                   * for standard hostdev (NB: when there is a network
>                   * with <forward mode='hostdev', there will need to be
>                   * code here that adds the newly minted hostdev to the
>                   * hostdevs array).
>                   */
> +                if (qemuAssignDeviceHostdevAlias(def,
> +                                                 hostdev,

Combine the above two lines.

> +                                                 (def->nhostdevs-1)) < 0) {
> +                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                    _("Could not assign alias to Net Hostdev"));
> +                    goto error;
> +                }
> +                
> +                if (virDomainHostdevFind(def,
> +                                         hostdev,
> +                                         &found) < 0) {


If the device is found already on the list, you should log an error and
fail.


> +                    if (virDomainHostdevInsert(def, 
> +                                               hostdev) < 0) {
> +                        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                        _("Hostdev not inserted into the array"));
> +                        goto error;
> +                    }
> +                    if (qemuPrepareHostdevPCIDevices(driver, def->name, def->uuid, 
> +                                                     &hostdev, 1) < 0) {
> +                        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                        _("Prepare Hostdev PCI Devices failed"));
> +                        goto error;

It took me awhile to follow that trail, but I do finally understand that
this is necessary (because qemuPrepareHostDevices has already been
called by the time we get to here and are building the qemu commandline).

> +                    } 
> +                }
>                  continue;
>              }
>  

ACK with a better commit log message, fixing the comment in the code,
and logging an error if the device is found already on the hostdev list.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list