Re: [libvirt] [PATCH 4/6] qemu: qemuMonitorQueryRxFilter - retrieve guest netdev rx-filter

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)