12:48 p.m.
On 02/20/2012 05:40 PM, Eric Blake wrote:
On 02/20/2012 10:10 AM, Laine Stump wrote:
> This is the new interface type that sets up a PCI/USB network device
> to be assigned to the guest with PCI/USB passthrough after
> initializing some network device-specific things from the config
> (e.g. MAC address, virtualport profile parameters). Here is an example
> of the syntax:
>
> <interface type='hostdev' managed='yes'>
> <source>
> <address type='pci' domain='0' bus='0'
slot='4' function='0'/>
> </source>
> <mac address='00:11:22:33:44:55'/>
> <address type='pci' domain='0' bus='0'
slot='7' function='0'/>
> </interface>
>
> This would assign the PCI card from bus 0 slot 4 function 0 on the
> host, to bus 0 slot 7 function 0 on the guest, but would first set the
> MAC address of the card to 00:11:22:33:44:55.
>
> Although it's not expected to be used very much, usb network hostdevs
> are also supported for completeness.
Even for common things like USB wifi sticks? (Hmm, I've got one of
those lying around - might be fun to play with :)
Oh sure, usb network devices exist (I've got a >10 year old USB1 wired
ethernet adapter hanging off the back of my desktop system just for the
fun of it :-), it just seems doubtful anyone using one of those devices
would have a need for either 1) setting a difference MAC address than
what's in the hardware, or 2) associating it with a vepa/vnlink-capable
switch. And if that's the case, it's probably just as well to use a
standard <hostdev> entry to assign it to the guest (or use one of the
emulated network devices). Mainly I'm just including it for consistency
with <hostdev>.
> @@ -966,6 +967,12 @@
virDomainActualNetDefFree(virDomainActualNetDefPtr def)
> VIR_FREE(def->data.direct.linkdev);
> VIR_FREE(def->data.direct.virtPortProfile);
> break;
> + case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> + /* currently there is nothing in a virDomainHostdevDef
> + * that requires freeing.
> + */
> + VIR_FREE(def->data.hostdev.virtPortProfile);
Is that comment still accurate? (Two instances)
Yes, it is accurate. For a non-embedded hostdevdef, there is just an
info that needs to be freed. For an embedded hostdevdef (which this is
by definition), the info is just a pointer to the parent device object's
info, so there is nothing to clear.
I guess in terms of future maintenance, this could be problematic
though. You've convinced me to add a virDomainHostdefDefClear() function
and call it in each of these cases. That way if somebody ever does add
something that needs freeing, they'll hopefully do the freeing in the
...Clear() function and we'll be covered.
Of course that function should be a part of Patch 1/6, so I'll have to
re-spin it...
> @@ -4082,6 +4100,31 @@ virDomainActualNetDefParseXML(xmlNodePtr
node,
> (!(actual->data.direct.virtPortProfile =
> virNetDevVPortProfileParse(virtPortNode))))
> goto error;
> + } else if (actual->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> + xmlNodePtr virtPortNode = virXPathNode("./virtualport", ctxt);
> + virDomainHostdevDefPtr hostdev = &actual->data.hostdev.def;
> +
> + hostdev->parent.type = VIR_DOMAIN_DEVICE_NET;
> + hostdev->parent.data.net = parent;
> + hostdev->info = &parent->info;
Might be some tweaks here if you take my comments on 5/6 to have hostdev
track just a pointer, rather than a full parent struct.
As I said in my response to the 5/6 review, I really prefer to leave it
as it is, since a virDomainDeviceDef really is just a pointer + a single
int anyway.
> + /* The helper function expects type to already be found
and
> + * passed in as a string, since it is in a different place in
> + * NetDef vs HostdevDef.
> + */
> + addrtype = virXPathString("string(./source/address/@type)",
ctxt);
> + if ((!addrtype) && virXPathNode("./source/vendor", ctxt))
> + addrtype = strdup("usb"); /* source/vendor implies usb device
*/
Indentation, and check for OOM.
Yeah, I suppose that would be more informative than just relying on the
error log in virDomainHostdevPartsParse() (which would just say that
address type is missing).
(NB: virXPathString(), which is called here to try and get the type from
./source/address/@type, will log an error message and return NULL if it
encounteres OOM, but if the attribute just isn't in the document it will
return,... , well, it will also return a NULL in that case too. So
although a message is logged, the current operation isn't aborted. Don't
know if that's worth doing anything about...)
> @@ -4397,6 +4446,27 @@ virDomainNetDefParseXML(virCapsPtr caps,
>
> break;
>
> + case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> + hostdev = &def->data.hostdev.def;
> + hostdev->parent.type = VIR_DOMAIN_DEVICE_NET;
> + hostdev->parent.data.net = def;
> + hostdev->info = &def->info;
> + /* The helper function expects type to already be found and
> + * passed in as a string, since it is in a different place in
> + * NetDef vs HostdevDef.
> + */
> + addrtype = virXPathString("string(./source/address/@type)",
ctxt);
> + if ((!addrtype) && virXPathNode("./source/vendor", ctxt))
> + addrtype = strdup("usb"); /* source/vendor implies usb device
*/
Again.
> + case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> + virBufferAdjustIndent(buf, 8);
> + if (virDomainHostdevSourceFormat(buf, &def->data.hostdev.def,
> + flags, true) < 0) {
> + return -1;
> + }
> + if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile,
> + buf) < 0) {
> + return -1;
> + }
> + virBufferAdjustIndent(buf, -8);
> + break;
> +
> +
> + if (virNetDevVPortProfileFormat(def->data.hostdev.virtPortProfile, buf)
< 0)
> + goto error;
> + virBufferAdjustIndent(buf, -8);
Oops - duplicated code (two profile prints, and two unindents but only
one indent).
Strange. I really would like to attribute that to git rebase doing
something odd, as I'd like to believe I didn't actually write it that
way, but I might have accidentally hit the past button twice or something.
> +virDomainHostdevDefPtr
> +virDomainNetGetActualHostdev(virDomainNetDefPtr iface)
> +{
> + if (iface->type == VIR_DOMAIN_NET_TYPE_HOSTDEV)
> + return &iface->data.hostdev.def;
> + else if ((iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) &&
> + (iface->data.network.actual->type
> + == VIR_DOMAIN_NET_TYPE_HOSTDEV)) {
Style: you have 'if ... else if { ... }' with mismatched bracing.
Hmm, since the first if just calls return, you can s/else //, and you've
fixed the style problem without having to touch {}.
Yep. That entire function was a last minute addition, and I didn't spend
as much time worrying over it as normal...