On 08/14/2012 07:44 AM, Laine Stump wrote:
> 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.
The device will be found on the list when using interface type=hostdev.
If I log an error and fail wouldn't that mean that interface type=hostdev will always fail
at this point?
>
>
>> + 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list