On 09/24/2014 05:50 AM, Laine Stump wrote:

This new attribute will control whether or not libvirt will pay attention to guest notifications about changes to network device mac addresses and receive filters. The default for this is 'no' (for security reasons). If it is set to 'yes' *and* the specified device model and connection support it (currently only macvtap+virtio) then libvirt will watch for NIC_RX_FILTER_CHANGED events, and when it receives one, it will issue a query-rx-filter command, retrieve the result, and modify the host-side macvtap interface's mac address and unicast/multicast filters accordingly.

The functionality behind this attribute will be in a later patch. This patch merely adds the attribute to the top-level of a domain's <interface> as well as to <network> and <portgroup>, and adds documentation and schema/xml2xml tests. Rather than adding even more test files, I've just added the net attribute in various applicable places of existing test files. --- docs/formatdomain.html.in | 38 ++++++++++++++++---- docs/formatnetwork.html.in | 28 +++++++++++++-- docs/schemas/domaincommon.rng | 5 +++ docs/schemas/network.rng | 10 ++++++ src/conf/domain_conf.c | 42 ++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++ src/conf/network_conf.c | 35 ++++++++++++++++++ src/conf/network_conf.h | 2 ++ src/libvirt_private.syms | 1 + tests/networkxml2xmlin/vepa-net.xml | 4 +-- tests/networkxml2xmlout/vepa-net.xml | 4 +-- .../qemuxml2argv-net-virtio-network-portgroup.xml | 4 +-- 12 files changed, 160 insertions(+), 16 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index eefdd5e..17e180d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3343,10 +3343,9 @@ <pre> ... <devices> - <interface type='bridge'> - <source bridge='xenbr0'/> - <mac address='00:16:3e:5d:c7:9e'/> - <script path='vif-bridge'/> + <interface type='direct' trustGuestRxFilters='yes'> + <source dev='eth0'/> + <mac address='52:54:00:5d:c7:9e'/> <boot order='1'/> <rom bar='off'/> </interface> @@ -3356,8 +3355,21 @@ <p> There are several possibilities for specifying a network interface visible to the guest. Each subsection below provides - more details about common setup options. Additionally, - each <code><interface></code> element has an + more details about common setup options. + </p> + <p> + libvirt allows specifying when the host should trust reports + from the guest of changes to the interface mac address and + receive filters by setting the <code>trustGuestRxFilters</code> + attribute to <code>yes</code><span class="since">Since + 1.2.9</span>. <code>trustGuestRxFilters</code> defaults + to <code>no</code> for security reasons, and support depends on + the guest network device driver as well as the type of + connection on the host - currently it is only supported for the + virtio driver, and for macvtap connections on the host.

<span class="since">Since 1.2.9</span>), the <code>interface</code> element property <code>trustGuestRxFilters</code> provides the capability for the host to detect and trust reports from the guest regarding changes to the interface mac address and receive filters by setting the attribute to <code>yes</yes>. The default setting for the attribute is <code>no</code> for security reasons and support depends on the guest network device driver as well as the type of connection on the host - currently it is only supported for the virtio driver and for macvtap connections on the host. [just looks wierd starting with libvirt allows... I also changed the "virtio driver, and for" to not have the "," since it's just joining two phrases not a list of things...]

+ </p> + <p> + Each <code><interface></code> element has an optional <code><address></code> sub-element that can tie the interface to a particular pci slot, with attribute <code>type='pci'</code> @@ -3589,6 +3601,18 @@ being the default mode. The individual modes cause the delivery of packets to behave as follows: </p> + <p> + If the model type is set to <code>virtio</code> and + interface's <code>trustGuestRxFilters</code> attribute is set + to <code>yes</code>, changes made to the interface mac address, + unicast/multicast receive filters, and vlan settings in the + guest will be monitored and propogated to the associated macvtap

s/propogated/propagated/

+ device on the host. <span class="since">Since + 1.2.9</span>. If <code>trustGuestRxFilters</code> is not set, or

Instead of "...on the host. Since 1.2.9. If..." - consider "...on the host (since 1.2.9). If..."

+ is not supported for the device model in use, an attempted + change to the mac address originating from the guest side will + result in a non-working network connection. + </p>

<dl> <dt><code>vepa</code></dt> @@ -3621,7 +3645,7 @@ ... <devices> ... - <interface type='direct'> + <interface type='direct' trustGuestRxFilters='no'> <source dev='eth0' mode='vepa'/> </interface> </devices> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 1a8ad8e..ff73952 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -35,7 +35,7 @@ </p>

<pre> - <network ipv6='yes'> + <network ipv6='yes' trustGuestRxFilters='no'> <name>default</name> <uuid>3e3fce45-4f53-4fa7-bb32-11f34168b82b</uuid> ...</pre> @@ -60,6 +60,16 @@ to have guest-to-guest communications. For further information, see the example below for the example with no gateway addresses. <span class="since">Since 1.0.1</span></dd> + <dt><code>trustGuestRxFilters='yes'</code></dt> + <dd>The optional parameter <code>trustGuestRxFilters</code> can + be used to set that attribute of the same name for each domain + interface connected to this network <span class="since">Since + 1.2.9</span> See

I think the "Since" needs some parentheses and period before the "See", eg (<span class="since">since 1.2.9</span>). See

+ the <a href="formatdomain.html#elementSNICS">Network + interfaces</a> section of the domain XML documentation for + more details. Note that an explicit setting of this attribute + in a portgroup or the individual domain interface will + override the setting in the network.</dd> </dl>

<h3><a name="elementsConnect">Connectivity</a></h3> @@ -606,7 +616,7 @@ <outbound average='1000' peak='5000' burst='5120'/> </bandwidth> </portgroup></b> - <b><portgroup name='sales'> + <b><portgroup name='sales' trustGuestRxFilters='no'> <virtualport type='802.1Qbh'> <parameters profileid='salestest'/> </virtualport> @@ -626,7 +636,7 @@ network can have multiple portgroup elements (and one of those can optionally be designated as the 'default' portgroup for the network), and each portgroup has a name, as well as various - subelements associated with it. The currently supported + attributes and subelements associated with it. The currently supported subelements are <code><bandwidth></code> (described <a href="formatnetwork.html#elementQoS">here</a>) and <code><virtualport></code> @@ -650,6 +660,18 @@ considered an error, and will prevent the interface from starting. </p> + <p> + portgroups also support the optional + parameter <code>trustGuestRxFilters</code> which can be used to + set that attribute of the same name for each domain interface + using this portgroup <span class="since">Since 1.2.9</span> See

Similar issue here w/r/t to Since & See.

+ the <a href="formatdomain.html#elementSNICS">Network + interfaces</a> section of the domain XML documentation for more + details. Note that an explicit setting of this attribute in the + portgroup overrides the network-wide setting, and an explicit + setting in the individual domain interface will override the + setting in the portgroup. + </p>

<h5><a name="elementsStaticroute">Static Routes</a></h5> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b6b309d..74a6d8c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2007,6 +2007,11 @@ --> <define name="interface"> <element name="interface"> + <optional> + <attribute name="trustGuestRxFilters"> + <ref name="virYesNo"/> + </attribute> + </optional>

Shouldn't this go after the end of the <choice>? Since that's how it's written out in the xml.

<choice> <group> <attribute name="type"> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index dc732b4..4546f80 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -24,6 +24,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="trustGuestRxFilters"> + <ref name="virYesNo"/> + </attribute> + </optional> <interleave>

<!-- The name of the network, used to refer to it through the API @@ -197,6 +202,11 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <attribute name="trustGuestRxFilters"> + <ref name="virYesNo"/> + </attribute> + </optional> <interleave> <optional> <ref name="virtualPortProfile"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9cc118c..7e0d147 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6756,6 +6756,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, char *type = NULL; char *mode = NULL; char *addrtype = NULL; + char *trustGuestRxFilters = NULL;

if (VIR_ALLOC(actual) < 0) return -1; @@ -6783,6 +6784,16 @@ virDomainActualNetDefParseXML(xmlNodePtr node, goto error; }

+ trustGuestRxFilters = virXMLPropString(node, "trustGuestRxFilters"); + if (trustGuestRxFilters && + ((int)(actual->trustGuestRxFilters + = virTristateBoolTypeFromString(trustGuestRxFilters)) <= 0)) {

No need for the (int) cast.

+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown trustGuestRxFilters value '%s'"), + trustGuestRxFilters); + goto error; + } + virtPortNode = virXPathNode("./virtualport", ctxt); if (virtPortNode) { if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE || @@ -6869,6 +6880,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, VIR_FREE(type); VIR_FREE(mode); VIR_FREE(addrtype); + VIR_FREE(trustGuestRxFilters); virDomainActualNetDefFree(actual);

ctxt->node = save_ctxt; @@ -6919,6 +6931,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *vhostuser_mode = NULL; char *vhostuser_path = NULL; char *vhostuser_type = NULL; + char *trustGuestRxFilters = NULL; virNWFilterHashTablePtr filterparams = NULL; virDomainActualNetDefPtr actual = NULL; xmlNodePtr oldnode = ctxt->node; @@ -6940,6 +6953,16 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, def->type = VIR_DOMAIN_NET_TYPE_USER; }

+ trustGuestRxFilters = virXMLPropString(node, "trustGuestRxFilters"); + if (trustGuestRxFilters && + ((int)(def->trustGuestRxFilters + = virTristateBoolTypeFromString(trustGuestRxFilters)) <= 0)) {

No need for the (int) cast.

+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown trustGuestRxFilters value '%s'"), + trustGuestRxFilters); + goto error; + } + cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { @@ -7469,6 +7492,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(mode); VIR_FREE(linkstate); VIR_FREE(addrtype); + VIR_FREE(trustGuestRxFilters); virNWFilterHashTableFree(filterparams);

return def; @@ -16491,6 +16515,9 @@ virDomainActualNetDefFormat(virBufferPtr buf, if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); } + if (def->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " trustGuestRxFilters='%s'", + virTristateBoolTypeToString(def->trustGuestRxFilters));

Showoff - although other places that use the tristate logic just use "if (def->trustGuestRxFilters)" (since BOOL_ABSENT == 0)

virBufferAddLit(buf, ">\n");

virBufferAdjustIndent(buf, 2); @@ -16575,6 +16602,9 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<interface type='%s'", typeStr); if (hostdef && hostdef->managed) virBufferAddLit(buf, " managed='yes'"); + if (def->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " trustGuestRxFilters='%s'", + virTristateBoolTypeToString(def->trustGuestRxFilters));

Similarly here... Also I noted that the domaincommon.rng file has the "trustGuestRxFilters" defined first followed by type, managed, etc., while this code has it the other way (eg, last). I thought that caused issues for some test, but I guess it didn't. Maybe we just got lucky.

virBufferAddLit(buf, ">\n");

virBufferAdjustIndent(buf, 2); @@ -19976,6 +20006,18 @@ virDomainNetGetActualVlan(virDomainNetDefPtr iface) return NULL; }

+ +bool +virDomainNetGetActualTrustGuestRxFilters(virDomainNetDefPtr iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK && + iface->data.network.actual) + return (iface->data.network.actual->trustGuestRxFilters + == VIR_TRISTATE_BOOL_YES); + return iface->trustGuestRxFilters == VIR_TRISTATE_BOOL_YES; +} + + /* Return listens[i] from the appropriate union for the graphics * type, or NULL if this is an unsuitable type, or the index is out of * bounds. If force0 is TRUE, i == 0, and there is no listen array, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d97b1f8..2d0cb06 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -880,6 +880,7 @@ struct _virDomainActualNetDef { virNetDevVPortProfilePtr virtPortProfile; virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; + int trustGuestRxFilters; /* enum virTristateBool */ unsigned int class_id; /* class ID for bandwidth 'floor' */ };

@@ -954,6 +955,7 @@ struct _virDomainNetDef { virNWFilterHashTablePtr filterparams; virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; + int trustGuestRxFilters; /* enum virTristateBool */ int linkstate; };

@@ -2471,6 +2473,7 @@ virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface); virNetDevBandwidthPtr virDomainNetGetActualBandwidth(virDomainNetDefPtr iface); virNetDevVlanPtr virDomainNetGetActualVlan(virDomainNetDefPtr iface); +bool virDomainNetGetActualTrustGuestRxFilters(virDomainNetDefPtr iface);

int virDomainControllerInsert(virDomainDefPtr def, virDomainControllerDefPtr controller) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 892bd8a..63b5917 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1615,6 +1615,7 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def, xmlNodePtr vlanNode; xmlNodePtr bandwidth_node; char *isDefault = NULL; + char *trustGuestRxFilters = NULL;

int result = -1;

@@ -1632,6 +1633,18 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def, isDefault = virXPathString("string(./@default)", ctxt); def->isDefault = isDefault && STRCASEEQ(isDefault, "yes");

+ trustGuestRxFilters + = virXPathString("string(./@trustGuestRxFilters)", ctxt); + if (trustGuestRxFilters) { + if ((def->trustGuestRxFilters + = virTristateBoolTypeFromString(trustGuestRxFilters)) <= 0) {

You could combine into one if like was done in domain_conf.

+ virReportError(VIR_ERR_XML_ERROR, + _("Invalid trustGuestRxFilters setting '%s' " + "in portgroup"), trustGuestRxFilters); + goto cleanup; + } + } + virtPortNode = virXPathNode("./virtualport", ctxt); if (virtPortNode && (!(def->virtPortProfile = virNetDevVPortProfileParse(virtPortNode, 0)))) { @@ -1654,6 +1667,7 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def, virPortGroupDefClear(def); } VIR_FREE(isDefault); + VIR_FREE(trustGuestRxFilters);

ctxt->node = save; return result; @@ -2013,6 +2027,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) xmlNodePtr virtPortNode = NULL; xmlNodePtr forwardNode = NULL; char *ipv6nogwStr = NULL; + char *trustGuestRxFilters = NULL; xmlNodePtr save = ctxt->node; xmlNodePtr bandwidthNode = NULL; xmlNodePtr vlanNode; @@ -2061,6 +2076,20 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) VIR_FREE(ipv6nogwStr); }

+ trustGuestRxFilters + = virXPathString("string(./@trustGuestRxFilters)", ctxt); + if (trustGuestRxFilters) { + if ((def->trustGuestRxFilters + = virTristateBoolTypeFromString(trustGuestRxFilters)) <= 0) {

Ditto

+ virReportError(VIR_ERR_XML_ERROR, + _("Invalid trustGuestRxFilters setting '%s' " + "in network '%s'"), + trustGuestRxFilters, def->name); + goto error;

^^^ Memleak ...

+ } + VIR_FREE(trustGuestRxFilters);

Memory leak if the goto error occurs. Move VIR_FREE() to error:

+ } + /* Parse network domain information */ def->domain = virXPathString("string(./domain[1]/@name)", ctxt);

@@ -2597,6 +2626,9 @@ virPortGroupDefFormat(virBufferPtr buf, if (def->isDefault) { virBufferAddLit(buf, " default='yes'"); } + if (def->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " trustGuestRxFilters='%s'", + virTristateBoolTypeToString(def->trustGuestRxFilters));

Similar to domain_conf - the BOOL_ABSENT isn't necessary

virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); if (virNetDevVlanFormat(&def->vlan, buf) < 0) @@ -2675,6 +2707,9 @@ virNetworkDefFormatBuf(virBufferPtr buf, } if (def->ipv6nogw) virBufferAddLit(buf, " ipv6='yes'"); + if (def->trustGuestRxFilters != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, " trustGuestRxFilters='%s'", + virTristateBoolTypeToString(def->trustGuestRxFilters));

ditto Nothing major - just some minor cleanups and it's an ACK John

virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<name>%s</name>\n", def->name); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 7ed58cd..660cd2d 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -219,6 +219,7 @@ struct _virPortGroupDef { virNetDevVPortProfilePtr virtPortProfile; virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; + int trustGuestRxFilters; /* enum virTristateBool */ };

typedef struct _virNetworkDef virNetworkDef; @@ -256,6 +257,7 @@ struct _virNetworkDef { virPortGroupDefPtr portGroups; virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; + int trustGuestRxFilters; /* enum virTristateBool */ };

typedef struct _virNetworkObj virNetworkObj; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a339ced..bb2b9a3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -331,6 +331,7 @@ virDomainNetGetActualBridgeName; virDomainNetGetActualDirectDev; virDomainNetGetActualDirectMode; virDomainNetGetActualHostdev; +virDomainNetGetActualTrustGuestRxFilters; virDomainNetGetActualType; virDomainNetGetActualVirtPortProfile; virDomainNetGetActualVlan; diff --git a/tests/networkxml2xmlin/vepa-net.xml b/tests/networkxml2xmlin/vepa-net.xml index 030c1d1..07c59c5 100644 --- a/tests/networkxml2xmlin/vepa-net.xml +++ b/tests/networkxml2xmlin/vepa-net.xml @@ -1,4 +1,4 @@ -<network> +<network trustGuestRxFilters="no"> <name>vepa-net</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8b</uuid> <forward mode="vepa"> @@ -14,7 +14,7 @@ <parameters typeid="2193047" typeidversion="3"/> </virtualport> </portgroup> - <portgroup name="alice"> + <portgroup name="alice" trustGuestRxFilters="yes"> <virtualport type="802.1Qbg"> <parameters managerid="13"/> </virtualport> diff --git a/tests/networkxml2xmlout/vepa-net.xml b/tests/networkxml2xmlout/vepa-net.xml index 4d35a8a..b266620 100644 --- a/tests/networkxml2xmlout/vepa-net.xml +++ b/tests/networkxml2xmlout/vepa-net.xml @@ -1,4 +1,4 @@ -<network> +<network trustGuestRxFilters='no'> <name>vepa-net</name> <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8b</uuid> <forward dev='eth1' mode='vepa'> @@ -14,7 +14,7 @@ <parameters typeid='2193047' typeidversion='3'/> </virtualport> </portgroup> - <portgroup name='alice'> + <portgroup name='alice' trustGuestRxFilters='yes'> <virtualport type='802.1Qbg'> <parameters managerid='13'/> </virtualport> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml index 950a9db..6cba439 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml @@ -22,7 +22,7 @@ <controller type='usb' index='0'/> <controller type='ide' index='0'/> <controller type='pci' index='0' model='pci-root'/> - <interface type='network'> + <interface type='network' trustGuestRxFilters='yes'> <mac address='00:11:22:33:44:55'/> <source network='rednet' portgroup='bob'/> <vlan> @@ -33,7 +33,7 @@ </virtualport> <model type='virtio'/> </interface> - <interface type='network'> + <interface type='network' trustGuestRxFilters='no'> <mac address='10:11:22:33:44:55'/> <source network='blue' portgroup='sam'/> <virtualport>

On 09/24/2014 07:45 PM, John Ferlan wrote:

On 09/24/2014 05:50 AM, Laine Stump wrote:

...

This same structure will be used to retrieve RX filter info for interfaces on the host via netlink messages, and RX filter info for interfaces on the guest via the qemu "query-rx-filter" command. --- src/libvirt_private.syms | 8 +++++++ src/util/virnetdev.c | 40 +++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 57 +++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bb2b9a3..e5723d2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1595,6 +1595,14 @@ virNetDevReplaceMacAddress; virNetDevReplaceNetConfig; virNetDevRestoreMacAddress; virNetDevRestoreNetConfig; +virNetDevRxFilterFree; +virNetDevRxFilterMulticastModeTypeFromString; +virNetDevRxFilterMulticastModeTypeToString; +virNetDevRxFilterNew; +virNetDevRxFilterUnicastModeTypeFromString; +virNetDevRxFilterUnicastModeTypeToString; +virNetDevRxFilterVlanModeTypeFromString; +virNetDevRxFilterVlanModeTypeToString; virNetDevSetIPv4Address; virNetDevSetMAC; virNetDevSetMTU; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 8815e18..dd1f530 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1932,3 +1932,43 @@ virNetDevGetLinkInfo(const char *ifname, return 0; } #endif /* defined(__linux__) */ + + +VIR_ENUM_IMPL(virNetDevRxFilterUnicastMode, + VIR_NETDEV_RX_FILTER_UNICAST_MODE_LAST, + "none", + "normal"); + +VIR_ENUM_IMPL(virNetDevRxFilterMulticastMode, + VIR_NETDEV_RX_FILTER_MULTICAST_MODE_LAST, + "none", + "normal"); + +VIR_ENUM_IMPL(virNetDevRxFilterVlanMode, + VIR_NETDEV_RX_FILTER_VLAN_MODE_LAST, + "none", + "normal"); Is there ever a chance these could be different for one of these types?

I looked back through my IRC discussions with Vlad (who worked on the qemu/kernel parts of this) and found that, while initially he thought they might have different values, that after looking it up in the qemu source he found that they have the same potential values. But I had already forgotten that by the time I got around to the implementation, and had put in three different enums simply because it's easier to combine them later than to split them out into separate enums when I find a difference. I've just now verified that all three have the same possible values according to qemu's qmp-commands.hx, so I will combine them into a single enum. BTW, I also see that in qemu, the associated enum is called RxState, but I think it is really more of a mode, and it's only related to the RX filters, so I'm still going to name it virNetDevRxFilterMode rather than the less descriptive virNetDevRxState.

Seems "excessive" to make 3 specific definitions when 1 generic one could suffice (drop the "Unicast, Multicast, Vlan")

...

+ + +virNetDevRxFilterPtr +virNetDevRxFilterNew(void) +{ + virNetDevRxFilterPtr filter; + + if (VIR_ALLOC(filter) < 0) + return NULL; + return filter; +} + + +void +virNetDevRxFilterFree(virNetDevRxFilterPtr filter) +{ + if (filter) { + VIR_FREE(filter->name); + VIR_FREE(filter->unicast.table); + VIR_FREE(filter->multicast.table); + VIR_FREE(filter->vlan.table); I haven't peeked ahead yet, but are these tables where the allocation is allocate space for 10 table entries, allocate entries in each array entry... If so, then the free will need to run through the tables.

They are tables, but each entry is a virMacAddr, *not* a virMacAddrPtr (if that was the case, the definition would say "virMacAddrPtr *table"), so there is no extra allocation.

Reason why I ask is I see size_t's for each structure indicating to me it's a array of entries.

...

+ VIR_FREE(filter); + } +} diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 69e365e..307871c 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007-2013 Red Hat, Inc. + * Copyright (C) 2007-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -37,6 +37,57 @@ typedef struct ifreq virIfreq; typedef void virIfreq; # endif

+typedef enum { + VIR_NETDEV_RX_FILTER_UNICAST_MODE_NONE = 0, + VIR_NETDEV_RX_FILTER_UNICAST_MODE_NORMAL, + + VIR_NETDEV_RX_FILTER_UNICAST_MODE_LAST +} virNetDevRxFilterUnicastMode; +VIR_ENUM_DECL(virNetDevRxFilterUnicastMode) + +typedef enum { + VIR_NETDEV_RX_FILTER_MULTICAST_MODE_NONE = 0, + VIR_NETDEV_RX_FILTER_MULTICAST_MODE_NORMAL, + + VIR_NETDEV_RX_FILTER_MULTICAST_MODE_LAST +} virNetDevRxFilterMulticastMode; +VIR_ENUM_DECL(virNetDevRxFilterMulticastMode) + +typedef enum { + VIR_NETDEV_RX_FILTER_VLAN_MODE_NONE = 0, + VIR_NETDEV_RX_FILTER_VLAN_MODE_NORMAL, + + VIR_NETDEV_RX_FILTER_VLAN_MODE_LAST +} virNetDevRxFilterVlanMode; +VIR_ENUM_DECL(virNetDevRxFilterVlanMode) Same here with respect to generic rather than specific

...

+ +typedef struct _virNetDevRxFilter virNetDevRxFilter; +typedef virNetDevRxFilter *virNetDevRxFilterPtr; +struct _virNetDevRxFilter { + char *name; /* the alias used by qemu, *not* name used by guest */ + virMacAddr mac; + bool promiscuous; + bool broadcastAllowed; + + struct { + int mode; /* enum virNetDevRxFilterUnicastMode */ + bool overflow; + virMacAddrPtr table; + size_t nTable; + } unicast; + struct { + int mode; /* enum virNetDevRxFilterMulticastMode */ + bool overflow; + virMacAddrPtr table; + size_t nTable; + } multicast; + struct { + int mode; /* enum virNetDevRxFilterVlanMode */ + unsigned int *table; + size_t nTable; + } vlan; +}; + Each of the mode's would just use the generic FilterMode and the comments adjusted accordingly...

soft ACK with changes depending on future patches which I haven't peeked at yet.

John

...

int virNetDevSetupControl(const char *ifname, virIfreq *ifr) ATTRIBUTE_RETURN_CHECK; @@ -150,4 +201,8 @@ int virNetDevGetLinkInfo(const char *ifname, virInterfaceLinkPtr lnk) ATTRIBUTE_NONNULL(1);

+virNetDevRxFilterPtr virNetDevRxFilterNew(void) + ATTRIBUTE_RETURN_CHECK; +void virNetDevRxFilterFree(virNetDevRxFilterPtr filter); + #endif /* __VIR_NETDEV_H__ */

On 09/27/2014 12:40 AM, Amos Kong wrote:

On Wed, Sep 24, 2014 at 05:50:53AM -0400, Laine Stump wrote:

...

This same structure will be used to retrieve RX filter info for interfaces on the host via netlink messages, and RX filter info for interfaces on the guest via the qemu "query-rx-filter" command. --- src/libvirt_private.syms | 8 +++++++ src/util/virnetdev.c | 40 +++++++++++++++++++++++++++++++++ src/util/virnetdev.h | 57 +++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bb2b9a3..e5723d2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1595,6 +1595,14 @@ virNetDevReplaceMacAddress; virNetDevReplaceNetConfig; virNetDevRestoreMacAddress; virNetDevRestoreNetConfig; +virNetDevRxFilterFree; +virNetDevRxFilterMulticastModeTypeFromString; +virNetDevRxFilterMulticastModeTypeToString; +virNetDevRxFilterNew; +virNetDevRxFilterUnicastModeTypeFromString; +virNetDevRxFilterUnicastModeTypeToString; +virNetDevRxFilterVlanModeTypeFromString; +virNetDevRxFilterVlanModeTypeToString; The RxFilter ModeTypes of unicast, multicast, vlan are same

'normal', 'none', 'all'

Can we just use some general functions to process the string?

virNetDevRxFilterModeTypeFromString; virNetDevRxFilterModeTypeToString;

Yep, I've already made that change for V2. I had madde separe enums in the beginning because it wasn't yet clear to me that they would have identical possible values.

On 09/24/2014 05:50 AM, Laine Stump wrote:

This function can be called at any time to get the current status of a guest's network device rx-filter. In particular it is useful to call after libvirt recieves a NIC_RX_FILTER_CHANGED event - this event only tells you that something has changed in the rx-filter, the details are retrieved with the query-rx-filter monitor command (only available in the json monitor). The commend sent to the qemu monitor looks like this:

{"execute":"query-rx-filter", "arguments": {"name":"net2"} }'

and the results will look something like this:

{ "return": [ { "promiscuous": false, "name": "net2", "main-mac": "52:54:00:98:2d:e3", "unicast": "normal", "vlan": "normal", "vlan-table": [ 42, 0 ], "unicast-table": [

], "multicast": "normal", "multicast-overflow": false, "unicast-overflow": false, "multicast-table": [ "33:33:ff:98:2d:e3", "01:80:c2:00:00:21", "01:00:5e:00:00:fb", "33:33:ff:98:2d:e2", "01:00:5e:00:00:01", "33:33:00:00:00:01" ], "broadcast-allowed": false } ], "id": "libvirt-14" }

This is all parsed from JSON into a virNetDevRxFilter object for easier consumption. (unicast-table is usually empty, but is also an array of mac addresses similar to multicast-table).

(NB: LIBNL_CFLAGS was added to tests/Makefile.am because virnetdev.h now includes util/virnetlink.h, which includes netlink/msg.h when appropriate. Without LIBNL_CFLAGS, gcc can't find that file (if libnl/netlink isn't available, LIBNL_CFLAGS will be empty and virnetlink.h won't try to include netlink/msg.h anyway).) --- src/qemu/qemu_monitor.c | 26 ++++++ src/qemu/qemu_monitor.h | 4 + src/qemu/qemu_monitor_json.c | 215 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + tests/Makefile.am | 3 + 5 files changed, 251 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c25f002..48cbe3e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2918,6 +2918,32 @@ int qemuMonitorRemoveNetdev(qemuMonitorPtr mon, }

+int +qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias, + virNetDevRxFilterPtr *filter) +{ + int ret = -1; + VIR_DEBUG("mon=%p alias=%s filter=%p", + mon, alias, filter); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + }

The "if (!mon->json)" usually goes here rather than later as an else. Just trying to be consistent with other functions I'm assuming at some point whomever calls this will check if the query-rx-filter command exits (eg, the qemu capability exists)... Again I haven't peeked ahead.

+ + + VIR_DEBUG("mon=%p, alias=%s", mon, alias); + + if (mon->json) + ret = qemuMonitorJSONQueryRxFilter(mon, alias, filter); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("query-rx-filter requires JSON monitor")); + return ret; +} + + int qemuMonitorGetPtyPaths(qemuMonitorPtr mon, virHashTablePtr paths) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 6b91e29..c37e36f 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -31,6 +31,7 @@ # include "virbitmap.h" # include "virhash.h" # include "virjson.h" +# include "virnetdev.h" # include "device_conf.h" # include "cpu/cpu.h"

@@ -622,6 +623,9 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon, int qemuMonitorRemoveNetdev(qemuMonitorPtr mon, const char *alias);

+int qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias, + virNetDevRxFilterPtr *filter); + int qemuMonitorGetPtyPaths(qemuMonitorPtr mon, virHashTablePtr paths);

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a3d7c2c..58007e6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3194,6 +3194,221 @@ int qemuMonitorJSONRemoveNetdev(qemuMonitorPtr mon, }

+static int +qemuMonitorJSONQueryRxFilterParse(virJSONValuePtr msg, + virNetDevRxFilterPtr *filter) +{ + int ret = -1; + const char *tmp; + virJSONValuePtr returnArray, entry, table, element; + int nTable; + size_t i; + virNetDevRxFilterPtr fil = virNetDevRxFilterNew(); + + if (!(returnArray = virJSONValueObjectGet(msg, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-rx-filter reply was missing return data")); + goto cleanup; + } + if (returnArray->type != VIR_JSON_TYPE_ARRAY) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-rx-filter return data was not an array")); + goto cleanup; + } + if (!(entry = virJSONValueArrayGet(returnArray, 0))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query -rx-filter returne data missing array element")); + goto cleanup; + } + + if (!fil) + goto cleanup; + + if (!(tmp = virJSONValueObjectGetString(entry, "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid name " + "in query-rx-filter response")); + goto cleanup; + } + if (VIR_STRDUP(fil->name, tmp) < 0) + goto cleanup; + if ((!(tmp = virJSONValueObjectGetString(entry, "main-mac"))) || + virMacAddrParse(tmp, &fil->mac) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'main-mac' " + "in query-rx-filter response")); + goto cleanup; + } + if (virJSONValueObjectGetBoolean(entry, "promiscuous", + &fil->promiscuous) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'promiscuous' " + "in query-rx-filter response")); + goto cleanup; + } + if (virJSONValueObjectGetBoolean(entry, "broadcast-allowed", + &fil->broadcastAllowed) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'broadcast-allowed' " + "in query-rx-filter response")); + goto cleanup; + } + + if ((!(tmp = virJSONValueObjectGetString(entry, "unicast"))) || + ((fil->unicast.mode + = virNetDevRxFilterUnicastModeTypeFromString(tmp)) < 0)) {

Unless different values for each could be returned - I think the common function would work - requires change here...

+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'unicast' " + "in query-rx-filter response")); + goto cleanup; + } + if (virJSONValueObjectGetBoolean(entry, "unicast-overflow", + &fil->unicast.overflow) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'unicast-overflow' " + "in query-rx-filter response")); + goto cleanup; + } + if ((!(table = virJSONValueObjectGet(entry, "unicast-table"))) || + ((nTable = virJSONValueArraySize(table)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'unicast-table' array " + "in query-rx-filter response")); + goto cleanup; + } + if (VIR_ALLOC_N(fil->unicast.table, nTable)) + goto cleanup; + for (i = 0; i < nTable; i++) { + if (!(element = virJSONValueArrayGet(table, i)) || + !(tmp = virJSONValueGetString(element))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid element %zu of 'unicast' " + "list in query-rx-filter response"), i); + goto cleanup; + } + if (virMacAddrParse(tmp, &fil->unicast.table[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid mac address '%s' in 'unicast-table' " + "array in query-rx-filter response"), tmp); + goto cleanup; + } + } + fil->unicast.nTable = nTable;

hmm.. so this is what relates to patch 3's query... Looks like a [n] entry table of virMacAddr where each entry just gets copied in place - so the single VIR_FREE should be fine. No issue - just thinking outloud to help me make a better ACK for patch3

+ + if ((!(tmp = virJSONValueObjectGetString(entry, "multicast"))) || + ((fil->multicast.mode + = virNetDevRxFilterMulticastModeTypeFromString(tmp)) < 0)) {

Unless different values for each could be returned - I think the common function would work - requires change here...

+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'multicast' " + "in query-rx-filter response")); + goto cleanup; + } + if (virJSONValueObjectGetBoolean(entry, "multicast-overflow", + &fil->multicast.overflow) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'multicast-overflow' " + "in query-rx-filter response")); + goto cleanup; + } + if ((!(table = virJSONValueObjectGet(entry, "multicast-table"))) || + ((nTable = virJSONValueArraySize(table)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'multicast-table' array " + "in query-rx-filter response")); + goto cleanup; + } + if (VIR_ALLOC_N(fil->multicast.table, nTable)) + goto cleanup; + for (i = 0; i < nTable; i++) { + if (!(element = virJSONValueArrayGet(table, i)) || + !(tmp = virJSONValueGetString(element))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid element %zu of 'multicast' " + "list in query-rx-filter response"), i); + goto cleanup; + } + if (virMacAddrParse(tmp, &fil->multicast.table[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid mac address '%s' in 'multicast-table' " + "array in query-rx-filter response"), tmp); + goto cleanup; + } + } + fil->multicast.nTable = nTable; + + if ((!(tmp = virJSONValueObjectGetString(entry, "vlan"))) || + ((fil->vlan.mode + = virNetDevRxFilterVlanModeTypeFromString(tmp)) < 0)) {

Unless different values for each could be returned - I think the common function would work - requires change here...

+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'vlan' " + "in query-rx-filter response")); + goto cleanup; + } + if ((!(table = virJSONValueObjectGet(entry, "vlan-table"))) || + ((nTable = virJSONValueArraySize(table)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'vlan-table' array " + "in query-rx-filter response")); + goto cleanup; + } + if (VIR_ALLOC_N(fil->vlan.table, nTable)) + goto cleanup; + for (i = 0; i < nTable; i++) { + if (!(element = virJSONValueArrayGet(table, i)) || + virJSONValueGetNumberUint(element, &fil->vlan.table[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid element %zu of 'vlan-table' " + "array in query-rx-filter response"), i); + goto cleanup; + } + } + fil->vlan.nTable = nTable; + + ret = 0; + cleanup: + if (ret < 0) { + virNetDevRxFilterFree(fil); + fil = NULL; + } + *filter = fil; + return ret; +} + + +int +qemuMonitorJSONQueryRxFilter(qemuMonitorPtr mon, const char *alias, + virNetDevRxFilterPtr *filter) +{ + int ret = -1; + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-rx-filter", + "s:name", alias, + NULL); + virJSONValuePtr reply = NULL; + + if (!cmd) + goto cleanup; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONQueryRxFilterParse(reply, filter) < 0) + goto cleanup; + + ret = 0; + cleanup: + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + if (ret < 0) { + virNetDevRxFilterFree(*filter); + *filter = NULL; + } + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + /* * Example return data * diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d366179..a41281b 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -210,6 +210,9 @@ int qemuMonitorJSONAddNetdev(qemuMonitorPtr mon, int qemuMonitorJSONRemoveNetdev(qemuMonitorPtr mon, const char *alias);

+int qemuMonitorJSONQueryRxFilter(qemuMonitorPtr mon, const char *alias, + virNetDevRxFilterPtr *filter); + int qemuMonitorJSONGetPtyPaths(qemuMonitorPtr mon, virHashTablePtr paths);

diff --git a/tests/Makefile.am b/tests/Makefile.am index d6c3cfb..bd4371b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -35,6 +35,7 @@ AM_CFLAGS = \ -Dabs_builddir="\"$(abs_builddir)\"" \ -Dabs_srcdir="\"$(abs_srcdir)\"" \ $(LIBXML_CFLAGS) \ + $(LIBNL_CFLAGS) \ $(GNUTLS_CFLAGS) \ $(SASL_CFLAGS) \ $(SELINUX_CFLAGS) \ @@ -43,6 +44,8 @@ AM_CFLAGS = \ $(COVERAGE_CFLAGS) \ $(WARN_CFLAGS)

+ +

Not sure why there's extra lines here? Should be removed

AM_LDFLAGS = \ -export-dynamic

Seeing libnl copied into the Makefile just sends up the warning flag in general about libnl ACK (maybe Eric has a different thought about the Makefile) John

On Wed, Sep 24, 2014 at 05:50:54AM -0400, Laine Stump wrote:

This function can be called at any time to get the current status of a guest's network device rx-filter. In particular it is useful to call after libvirt recieves a NIC_RX_FILTER_CHANGED event - this event only tells you that something has changed in the rx-filter, the details are retrieved with the query-rx-filter monitor command (only available in the json monitor). The commend sent to the qemu monitor looks like this:

%s/commend/command/

{"execute":"query-rx-filter", "arguments": {"name":"net2"} }'

and the results will look something like this:

{ "return": [ { "promiscuous": false, "name": "net2", "main-mac": "52:54:00:98:2d:e3", "unicast": "normal", "vlan": "normal", "vlan-table": [ 42, 0 ], "unicast-table": [

], "multicast": "normal", "multicast-overflow": false, "unicast-overflow": false, "multicast-table": [ "33:33:ff:98:2d:e3", "01:80:c2:00:00:21", "01:00:5e:00:00:fb", "33:33:ff:98:2d:e2", "01:00:5e:00:00:01", "33:33:00:00:00:01" ], "broadcast-allowed": false } ], "id": "libvirt-14" }

This is all parsed from JSON into a virNetDevRxFilter object for easier consumption. (unicast-table is usually empty, but is also an array of mac addresses similar to multicast-table).

(NB: LIBNL_CFLAGS was added to tests/Makefile.am because virnetdev.h now includes util/virnetlink.h, which includes netlink/msg.h when appropriate. Without LIBNL_CFLAGS, gcc can't find that file (if libnl/netlink isn't available, LIBNL_CFLAGS will be empty and virnetlink.h won't try to include netlink/msg.h anyway).) --- src/qemu/qemu_monitor.c | 26 ++++++ src/qemu/qemu_monitor.h | 4 + src/qemu/qemu_monitor_json.c | 215 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + tests/Makefile.am | 3 + 5 files changed, 251 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c25f002..48cbe3e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2918,6 +2918,32 @@ int qemuMonitorRemoveNetdev(qemuMonitorPtr mon, }

+int +qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias, + virNetDevRxFilterPtr *filter) +{ + int ret = -1; + VIR_DEBUG("mon=%p alias=%s filter=%p", + mon, alias, filter); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + + VIR_DEBUG("mon=%p, alias=%s", mon, alias); + + if (mon->json) + ret = qemuMonitorJSONQueryRxFilter(mon, alias, filter); + else + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("query-rx-filter requires JSON monitor")); + return ret; +} + + int qemuMonitorGetPtyPaths(qemuMonitorPtr mon, virHashTablePtr paths) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 6b91e29..c37e36f 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -31,6 +31,7 @@ # include "virbitmap.h" # include "virhash.h" # include "virjson.h" +# include "virnetdev.h" # include "device_conf.h" # include "cpu/cpu.h"

@@ -622,6 +623,9 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon, int qemuMonitorRemoveNetdev(qemuMonitorPtr mon, const char *alias);

+int qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias, + virNetDevRxFilterPtr *filter); + int qemuMonitorGetPtyPaths(qemuMonitorPtr mon, virHashTablePtr paths);

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a3d7c2c..58007e6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3194,6 +3194,221 @@ int qemuMonitorJSONRemoveNetdev(qemuMonitorPtr mon, }

+static int +qemuMonitorJSONQueryRxFilterParse(virJSONValuePtr msg, + virNetDevRxFilterPtr *filter) +{ + int ret = -1; + const char *tmp; + virJSONValuePtr returnArray, entry, table, element; + int nTable; + size_t i; + virNetDevRxFilterPtr fil = virNetDevRxFilterNew(); + + if (!(returnArray = virJSONValueObjectGet(msg, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-rx-filter reply was missing return data")); + goto cleanup; + } + if (returnArray->type != VIR_JSON_TYPE_ARRAY) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-rx-filter return data was not an array")); + goto cleanup; + } + if (!(entry = virJSONValueArrayGet(returnArray, 0))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query -rx-filter returne data missing array element")); + goto cleanup; + } + + if (!fil) + goto cleanup;

How about checking fil after virNetDevRxFilterNew()?

+ + if (!(tmp = virJSONValueObjectGetString(entry, "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid name " + "in query-rx-filter response")); + goto cleanup; + } + if (VIR_STRDUP(fil->name, tmp) < 0) + goto cleanup; + if ((!(tmp = virJSONValueObjectGetString(entry, "main-mac"))) || + virMacAddrParse(tmp, &fil->mac) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'main-mac' " + "in query-rx-filter response")); + goto cleanup; + } + if (virJSONValueObjectGetBoolean(entry, "promiscuous", + &fil->promiscuous) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'promiscuous' " + "in query-rx-filter response")); + goto cleanup; + } + if (virJSONValueObjectGetBoolean(entry, "broadcast-allowed", + &fil->broadcastAllowed) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'broadcast-allowed' " + "in query-rx-filter response")); + goto cleanup; + } + + if ((!(tmp = virJSONValueObjectGetString(entry, "unicast"))) || + ((fil->unicast.mode + = virNetDevRxFilterUnicastModeTypeFromString(tmp)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'unicast' " + "in query-rx-filter response")); + goto cleanup; + } + if (virJSONValueObjectGetBoolean(entry, "unicast-overflow", + &fil->unicast.overflow) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'unicast-overflow' " + "in query-rx-filter response")); + goto cleanup; + } + if ((!(table = virJSONValueObjectGet(entry, "unicast-table"))) || + ((nTable = virJSONValueArraySize(table)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'unicast-table' array " + "in query-rx-filter response")); + goto cleanup; + } + if (VIR_ALLOC_N(fil->unicast.table, nTable)) + goto cleanup; + for (i = 0; i < nTable; i++) { + if (!(element = virJSONValueArrayGet(table, i)) || + !(tmp = virJSONValueGetString(element))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid element %zu of 'unicast' " + "list in query-rx-filter response"), i); + goto cleanup; + } + if (virMacAddrParse(tmp, &fil->unicast.table[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid mac address '%s' in 'unicast-table' " + "array in query-rx-filter response"), tmp); + goto cleanup; + } + } + fil->unicast.nTable = nTable; + + if ((!(tmp = virJSONValueObjectGetString(entry, "multicast"))) || + ((fil->multicast.mode + = virNetDevRxFilterMulticastModeTypeFromString(tmp)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'multicast' " + "in query-rx-filter response")); + goto cleanup; + } + if (virJSONValueObjectGetBoolean(entry, "multicast-overflow", + &fil->multicast.overflow) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'multicast-overflow' " + "in query-rx-filter response")); + goto cleanup; + } + if ((!(table = virJSONValueObjectGet(entry, "multicast-table"))) || + ((nTable = virJSONValueArraySize(table)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'multicast-table' array " + "in query-rx-filter response")); + goto cleanup; + } + if (VIR_ALLOC_N(fil->multicast.table, nTable)) + goto cleanup; + for (i = 0; i < nTable; i++) { + if (!(element = virJSONValueArrayGet(table, i)) || + !(tmp = virJSONValueGetString(element))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid element %zu of 'multicast' " + "list in query-rx-filter response"), i); + goto cleanup; + } + if (virMacAddrParse(tmp, &fil->multicast.table[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid mac address '%s' in 'multicast-table' " + "array in query-rx-filter response"), tmp); + goto cleanup; + } + } + fil->multicast.nTable = nTable; + + if ((!(tmp = virJSONValueObjectGetString(entry, "vlan"))) || + ((fil->vlan.mode + = virNetDevRxFilterVlanModeTypeFromString(tmp)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'vlan' " + "in query-rx-filter response")); + goto cleanup; + } + if ((!(table = virJSONValueObjectGet(entry, "vlan-table"))) || + ((nTable = virJSONValueArraySize(table)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing or invalid 'vlan-table' array " + "in query-rx-filter response")); + goto cleanup; + } + if (VIR_ALLOC_N(fil->vlan.table, nTable)) + goto cleanup; + for (i = 0; i < nTable; i++) { + if (!(element = virJSONValueArrayGet(table, i)) || + virJSONValueGetNumberUint(element, &fil->vlan.table[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid element %zu of 'vlan-table' " + "array in query-rx-filter response"), i); + goto cleanup; + } + } + fil->vlan.nTable = nTable; + + ret = 0; + cleanup: + if (ret < 0) { + virNetDevRxFilterFree(fil); + fil = NULL; + } + *filter = fil; + return ret; +} + + +int +qemuMonitorJSONQueryRxFilter(qemuMonitorPtr mon, const char *alias, + virNetDevRxFilterPtr *filter) +{ + int ret = -1; + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-rx-filter", + "s:name", alias, + NULL); + virJSONValuePtr reply = NULL; + + if (!cmd) + goto cleanup; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONQueryRxFilterParse(reply, filter) < 0) + goto cleanup; + + ret = 0; + cleanup: + if (ret == 0) + ret = qemuMonitorJSONCheckError(cmd, reply); + + if (ret < 0) { + virNetDevRxFilterFree(*filter); + *filter = NULL; + } + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + /* * Example return data * diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d366179..a41281b 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -210,6 +210,9 @@ int qemuMonitorJSONAddNetdev(qemuMonitorPtr mon, int qemuMonitorJSONRemoveNetdev(qemuMonitorPtr mon, const char *alias);

+int qemuMonitorJSONQueryRxFilter(qemuMonitorPtr mon, const char *alias, + virNetDevRxFilterPtr *filter); + int qemuMonitorJSONGetPtyPaths(qemuMonitorPtr mon, virHashTablePtr paths);

diff --git a/tests/Makefile.am b/tests/Makefile.am index d6c3cfb..bd4371b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -35,6 +35,7 @@ AM_CFLAGS = \ -Dabs_builddir="\"$(abs_builddir)\"" \ -Dabs_srcdir="\"$(abs_srcdir)\"" \ $(LIBXML_CFLAGS) \ + $(LIBNL_CFLAGS) \ $(GNUTLS_CFLAGS) \ $(SASL_CFLAGS) \ $(SELINUX_CFLAGS) \ @@ -43,6 +44,8 @@ AM_CFLAGS = \ $(COVERAGE_CFLAGS) \ $(WARN_CFLAGS)

+ + AM_LDFLAGS = \ -export-dynamic

-- 1.9.3

-- Amos.

On 09/24/2014 05:50 AM, Laine Stump wrote:

NIC_RX_FILTER_CHANGED is sent by qemu any time a NIC driver in the guest modified the NIC's RX Filter (for example, if the MAC address of the NIC is changed by the guest).

This patch doesn't do anything useful with that event; it just sets up all the plumbing to get news of the event into a worker thread with all proper locking/reference counting, and provide an easy place to add in desired functionality.

For easy reference the next time a handler for a qemu event is written, here is the sequence: I assume you mean qemu_monitor_json.c The handler in qemu_json_monitor.c calls

qemu_json_monitor.c:qemuMonitorJSONHandleNicRxFilterChanged()

which calls

qemu_monitor.c:qemuMonitorEmitNicRxFilterChanged()

which uses QEMU_MONITOR_CALLBACK() to call mon->cb->domainNicRxFilterChanged(), ie:

qemuProcessHandleNicRxFilterChanged()

which will queue an event QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED for a worker thread to handle.

When the worker thread gets the event, it calls

qemuProcessEventHandler()

which calls

processNicRxFilterChangedEvent()

and *that* is where the actual work will be done (and any event-specific memory allocated during qemuProcessHandleXXX() will be freed) - it is the middle of this function where functionality behind the event will be added in the next patch; for now there is just a VIR_DEBUG() to log the event. --- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 55 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 13 +++++++++++ src/qemu/qemu_monitor.h | 7 ++++++ src/qemu/qemu_monitor_json.c | 17 ++++++++++++++ src/qemu/qemu_process.c | 42 +++++++++++++++++++++++++++++++++ 6 files changed, 135 insertions(+)

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 845d3c7..ad45a66 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -195,6 +195,7 @@ typedef enum { QEMU_PROCESS_EVENT_WATCHDOG = 0, QEMU_PROCESS_EVENT_GUESTPANIC, QEMU_PROCESS_EVENT_DEVICE_DELETED, + QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED,

QEMU_PROCESS_EVENT_LAST } qemuProcessEventType; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 543de79..64f1d45 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4140,6 +4140,58 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver, }

+static void +processNicRxFilterChangedEvent(virQEMUDriverPtr driver, + virDomainObjPtr vm, + char *devAlias) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virDomainDeviceDef dev; + virDomainNetDefPtr def; + + VIR_DEBUG("Received NIC_RX_FILTER_CHANGED event for device %s " + "from domain %p %s", + devAlias, vm, vm->def->name); + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + VIR_DEBUG("Domain is not running"); + goto endjob; + } + + if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0) { + VIR_WARN("NIC_RX_FILTER_CHANGED event received for " + "non-existent device %s in domain %s", + devAlias, vm->def->name); + goto endjob; + } + if (dev.type != VIR_DOMAIN_DEVICE_NET) { + VIR_WARN("NIC_RX_FILTER_CHANGED event received for " + "non-network device %s in domain %s", + devAlias, vm->def->name); + goto endjob; + } I understand the need to check the device type, but is it necessary to log a warning message? I wonder if it may not cause unnecessary concern for someone viewing the logs. Is it possible to receive a NIC_RX_FILTER_CHANGED event for something other than a network device? + def = dev.data.net; + + /* handle the event - send query-rx-filter and respond to it. */ + + VIR_DEBUG("process NIC_RX_FILTER_CHANGED event for network " + "device %s in domain %s", def->info.alias, vm->def->name); + + endjob: + /* We had an extra reference to vm before starting a new job so ending the + * job is guaranteed not to remove the last reference. + */ + ignore_value(qemuDomainObjEndJob(driver, vm)); + + cleanup: + VIR_FREE(devAlias); + virObjectUnref(cfg); +} + + static void qemuProcessEventHandler(void *data, void *opaque) { struct qemuProcessEvent *processEvent = data; @@ -4160,6 +4212,9 @@ static void qemuProcessEventHandler(void *data, void *opaque) case QEMU_PROCESS_EVENT_DEVICE_DELETED: processDeviceDeletedEvent(driver, vm, processEvent->data); break; + case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED: + processNicRxFilterChangedEvent(driver, vm, processEvent->data); + break; case QEMU_PROCESS_EVENT_LAST: break; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 48cbe3e..a4661f8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1387,6 +1387,19 @@ qemuMonitorEmitDeviceDeleted(qemuMonitorPtr mon, }

+int +qemuMonitorEmitNicRxFilterChanged(qemuMonitorPtr mon, + const char *devAlias) +{ + int ret = -1; + VIR_DEBUG("mon=%p", mon); + + QEMU_MONITOR_CALLBACK(mon, ret, domainNicRxFilterChanged, mon->vm, devAlias); + + return ret; +} + + int qemuMonitorSetCapabilities(qemuMonitorPtr mon) { int ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c37e36f..e841505 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -171,6 +171,10 @@ typedef int (*qemuMonitorDomainDeviceDeletedCallback)(qemuMonitorPtr mon, virDomainObjPtr vm, const char *devAlias, void *opaque); +typedef int (*qemuMonitorDomainNicRxFilterChangedCallback)(qemuMonitorPtr mon, + virDomainObjPtr vm, + const char *devAlias, + void *opaque);

typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks; typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr; @@ -197,6 +201,7 @@ struct _qemuMonitorCallbacks { qemuMonitorDomainPMSuspendDiskCallback domainPMSuspendDisk; qemuMonitorDomainGuestPanicCallback domainGuestPanic; qemuMonitorDomainDeviceDeletedCallback domainDeviceDeleted; + qemuMonitorDomainNicRxFilterChangedCallback domainNicRxFilterChanged; };

char *qemuMonitorEscapeArg(const char *in); @@ -285,6 +290,8 @@ int qemuMonitorEmitPMSuspendDisk(qemuMonitorPtr mon); int qemuMonitorEmitGuestPanic(qemuMonitorPtr mon); int qemuMonitorEmitDeviceDeleted(qemuMonitorPtr mon, const char *devAlias); +int qemuMonitorEmitNicRxFilterChanged(qemuMonitorPtr mon, + const char *devAlias);

int qemuMonitorStartCPUs(qemuMonitorPtr mon, virConnectPtr conn); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 58007e6..640d96e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -81,6 +81,7 @@ static void qemuMonitorJSONHandleBalloonChange(qemuMonitorPtr mon, virJSONValueP static void qemuMonitorJSONHandlePMSuspendDisk(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleGuestPanic(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleDeviceDeleted(qemuMonitorPtr mon, virJSONValuePtr data); +static void qemuMonitorJSONHandleNicRxFilterChanged(qemuMonitorPtr mon, virJSONValuePtr data);

typedef struct { const char *type; @@ -96,6 +97,7 @@ static qemuEventHandler eventHandlers[] = { { "DEVICE_DELETED", qemuMonitorJSONHandleDeviceDeleted, }, { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, }, { "GUEST_PANICKED", qemuMonitorJSONHandleGuestPanic, }, + { "NIC_RX_FILTER_CHANGED", qemuMonitorJSONHandleNicRxFilterChanged, }, { "POWERDOWN", qemuMonitorJSONHandlePowerdown, }, { "RESET", qemuMonitorJSONHandleReset, }, { "RESUME", qemuMonitorJSONHandleResume, }, @@ -1041,6 +1043,21 @@ qemuMonitorJSONHandleDeviceDeleted(qemuMonitorPtr mon, virJSONValuePtr data) qemuMonitorEmitDeviceDeleted(mon, device); }

+ +static void +qemuMonitorJSONHandleNicRxFilterChanged(qemuMonitorPtr mon, virJSONValuePtr data) +{ + const char *name; + + if (!(name = virJSONValueObjectGetString(data, "name"))) { + VIR_WARN("missing device in NIC_RX_FILTER_CHANGED event"); + return; + } The device path is also sent with the event. It may be that data element is useless with regard to subsequent NIC_RX_FILTER_CHANGED event handling but, for my edification, I am curious as to why you ignored it. + + qemuMonitorEmitNicRxFilterChanged(mon, name); +} + + int qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon, const char *cmd_str, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index dddca35..07f1f38 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1489,6 +1489,47 @@ qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED, }

+static int +qemuProcessHandleNicRxFilterChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + const char *devAlias, + void *opaque) +{ + virQEMUDriverPtr driver = opaque; + struct qemuProcessEvent *processEvent = NULL; + char *data; + + virObjectLock(vm); + + VIR_DEBUG("Device %s RX Filter changed in domain %p %s", + devAlias, vm, vm->def->name); + + if (VIR_ALLOC(processEvent) < 0) + goto error; + + processEvent->eventType = QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED; + if (VIR_STRDUP(data, devAlias) < 0) + goto error; + processEvent->data = data; + processEvent->vm = vm; + + virObjectRef(vm); + if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) { + ignore_value(virObjectUnref(vm)); + goto error; + } + + cleanup: + virObjectUnlock(vm); + return 0; + error: + if (processEvent) + VIR_FREE(processEvent->data); + VIR_FREE(processEvent); + goto cleanup; +} + + static qemuMonitorCallbacks monitorCallbacks = { .eofNotify = qemuProcessHandleMonitorEOF, .errorNotify = qemuProcessHandleMonitorError, @@ -1510,6 +1551,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainPMSuspendDisk = qemuProcessHandlePMSuspendDisk, .domainGuestPanic = qemuProcessHandleGuestPanic, .domainDeviceDeleted = qemuProcessHandleDeviceDeleted, + .domainNicRxFilterChanged = qemuProcessHandleNicRxFilterChanged, };

static int

On 09/24/2014 10:19 PM, John Ferlan wrote:

On 09/24/2014 05:50 AM, Laine Stump wrote:

...

This patch fills in the functionality of processNicRxFilterChangedEvent(). It now checks if it is appropriate to respond to the NIC_RX_FILTER_CHANGED event (based on device type and configuration) and takes appropriate action. Currently it checks if the guest interface has been configured with trustGuestRxFilters='yes', and if the host side device is macvtap. If so, and the MAC address on the guest has changed, the MAC address of the macvtap device is changed to match.

The result of this is that networking from the guest will continue to work if the mac address of a macvtap-connected network device is changed from within the guest, as long as trustGuestRxFilters='yes' (previously changing the MAC address in the guest would break networking). --- src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 64f1d45..7801d91 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4146,8 +4146,13 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver, char *devAlias) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDeviceDef dev; virDomainNetDefPtr def; + virNetDevRxFilterPtr filter = NULL; + virMacAddr oldMAC; + char newMacStr[VIR_MAC_STRING_BUFLEN]; + int ret;

VIR_DEBUG("Received NIC_RX_FILTER_CHANGED event for device %s " "from domain %p %s", @@ -4175,11 +4180,55 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver, } def = dev.data.net;

+ if (!virDomainNetGetActualTrustGuestRxFilters(def)) { + VIR_WARN("ignore NIC_RX_FILTER_CHANGED event for network " + "device %s in domain %s", + def->info.alias, vm->def->name); So how often could this be emitted? Do we need some sort of "filter" to only message once per life of the domain?

As often as we respond to the previous NIC_RX_FILTER_CHANGED event - qemu won't send another event until we've issued a new query-rx-filter command for that same interface. So for domains where we've set trustGuestRxFilters='no', we will get exactly one event per interface. On domains where we trust the guest enough to use its filter info, we will never get a backlog of more than one per interface.

...

+ /* not sending "query-rx-filter" will also suppress any + * further NIC_RX_FILTER_CHANGED events for this device + */ + goto endjob; + } + /* handle the event - send query-rx-filter and respond to it. */

VIR_DEBUG("process NIC_RX_FILTER_CHANGED event for network " "device %s in domain %s", def->info.alias, vm->def->name);

There's no capabilities check necessary? What if the command doesn't exist in the underlying qemu?

If NIC_RX_FILTER_CHANGED exists, then query-rx-filter exists. If NIC_RX_FILTER_CHANGED doesn't exist, we will never get to this point.

...

+ qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorQueryRxFilter(priv->mon, devAlias, &filter); + qemuDomainObjExitMonitor(driver, vm); + if (ret < 0) + goto endjob; You filled in a lot of data from the qemuMonitorQueryRxFilter(), but you're only using filter->mac (e.g. main-mac).

Or am I missing something?

No, you're not missing anything. All the other information will be used in the future in a similar manner. This single patch was all that was necessary to get regular unicast traffic working after a mac address change, so it is useful on its own, but I didn't want to add in the utility functions half-finished.