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@xxxxxxxxxxxxxx>
> ---
> 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list