On 09/24/2014 08:11 PM, John Ferlan wrote:
>
> 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 not seeing that - every other function I checked is patterned just
like this one (which is a direct copy of qemuMonitorRemoveNetdev()).
>
> I'm assuming at some point whomever calls this will check if the
> query-rx-filter command exits (eg, the qemu capability exists)...
At one point I had added a capability bit for it, but since the command
is only issued in response to a NIC_RX_FILTER_CHANGED event, and the two
were implemented in qemu at the same time, we would never attempt to
call this function unless it was actually available in qemu, and I
didn't want to clutter up the capabilities with an extra item unless it
was really necessary.
>
> 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...
Yep. Changed it as discussed in 3/6.
>
>> + 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
Fell asleep with my thumb on the space bar??
>
>> AM_LDFLAGS = \
>> -export-dynamic
>>
>>
>
> Seeing libnl copied into the Makefile just sends up the warning flag in
> general about libnl
I was initially concerned as well, but keep in mind that it only affects
whether an include directory is added to the compile line, and that
doesn't happen for platforms without libnl.
>
> ACK (maybe Eric has a different thought about the Makefile)
So far I just removed the extra lines in the Makefile and changed the
names of the enum conversion functions.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list