[libvirt] [PATCH 7/7] Forward Mode 'Hostdev' qemu driver implementation

[Laine Stump]

I just found this review that I had typed up and not actually sent. I've
made the small changes and pushed the series in the meantime, but
figured I should send this just for archival purposes.


On 08/16/2012 11:42 AM, Shradha Shah wrote:
> For a network with <forward mode='hostdev', there is a need to
> add the newly minted hostdev to the hostdevs array.
>
> In this case we also call qemuPrepareHostDevicesas it has already been
> called by the time we get to here and are building the qemu commandline
>
> Signed-off-by: Shradha Shah <sshah at solarflare.com>
> ---
>  src/qemu/qemu_command.c |   39 +++++++++++++++++++++++++++++++++------
>  1 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6f6c6cd..1470edd 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) {
> -                /* 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 (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> +                    virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net);
> +                    virDomainHostdevDefPtr found;
> +                    /* For a network with <forward mode='hostdev', there is a need to 
> +                     * add the newly minted hostdev to the hostdevs array.
> +                     */
> +                    if (qemuAssignDeviceHostdevAlias(def, hostdev,
> +                                                     (def->nhostdevs-1)) < 0) {
> +                        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                       _("Could not assign alias to Net Hostdev"));

qemuAssignDeviceHostdevAlias logs its own errors, so I'm removing this
log message.

> +                        goto error;
> +                    }
> +                    
> +                    if (virDomainHostdevFind(def, hostdev, &found) < 0) {
> +                        if (virDomainHostdevInsert(def, hostdev) < 0) {
> +                            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                           _("Hostdev not inserted into the array"));


virDomainHostdevInsert *doesn't* log its own errors. However, the only
reason it can fail is if we're out of memory, so I'm replacing this log
with virReportOOMError().


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

This function also logs its own errors, so I'm removing this message
> +                            goto error;
> +                        } 
> +                    }
> +                    else {
> +                        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                       _("The Hostdev is being used by some other device"));

How about this instead? (by this time we're guaranteed that all of those
bits of info are valid)

                        virReportError(VIR_ERR_INTERNAL_ERROR,
                                       _("PCI device %04x:%02x:%02x.%x "
                                         "allocated from network %s is
already "
                                         "in use by domain %s"),
                                       hostdev->source.subsys.u.pci.domain,
                                       hostdev->source.subsys.u.pci.bus,
                                       hostdev->source.subsys.u.pci.slot,
                                      
hostdev->source.subsys.u.pci.function,
                                       net->data.network.name,
                                       def->name);


ACK with the log message changes. I'll squash them in before I push.