[ovs-dev] OVN: RFC add a new JSON-RPC selective monitoring method
Following the discussion on overcoming OVN scalability issues by allowing clients to monitor only rows that meet specific criteria (http://openvswitch.org/pipermail/dev/2015-August/059149.html), I propose to amend the OVSDB protocol specification (RFC 7047). Proposed amendment is to define a new monitor method, monitor_cond, for specifying monitoring conditions. Adding a new method allows keeping all of the existing client code intact, replacing code only in places that can benefit from this new method (e.g. in ovn-controller). Below is the description for a new JSON-RPC monitor_cond method to be added to the OVSDB protocol. (All references are to RFC 7047) monitor_cond: - The "monitor_cond" request enables a client to replicate subsets of tables within an OVSDB database by requesting notifications of changes to those rows in the table that match all the conditions specified in "where" by receiving these rows. The request object has the following members: * "method": "monitor_cond" * "params": [, , ] * "id": The parameter is used to match subsequent update notifications (see below) to this request. The object maps the name of the table to an array of . Each is an object with the following members: * "where": [*] optional * "select": optional is specified in Section 5.1 in the RFC is an object with the following members: * "initial": optional * "insert":optional * "delete":optional * "modify":optional The contents of this object specify how the columns or table are to be monitored, as explained in more detail below. The response object has the following members: * "result": * "error": null * "id": same "id" as request The object is described in detail in Section 4.1.6. It contains the contents of the tables for which "initial" rows are selected. If no tables' initial contents are requested, then "result" is an empty object. Subsequently, when changes to a specified table that match all the conditions in monitor-cond-request are committed, the changes are automatically sent to the client using the "update" monitor notification (see Section 4.1.6). This monitoring persists until the JSON-RPC session terminates or until the client sends a "monitor_cancel" JSON-RPC request. Each specifies one or more conditions and the manner in which the rows that match the conditions are to be monitored. The circumstances in which an "update" notification is sent for a row within the table are determined by : * If "initial" is omitted or true, every row in the table that matches the conditions is sent as part of the response to the "monitor_cond" request. * If "insert" is omitted or true, "update" notifications are sent for rows newly inserted into the table that match conditions. * If "delete" is omitted or true, "update" notifications are sent for rows deleted from the table that match conditions. * If "modify" is omitted or true, "update" notifications are sent whenever a row in the table that matches conditions is modified. ... I send this specification for review. Next step will be starting a design discussion for implementing this in the ovsdb-server. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/2] datapath-windows: Process tunnel filter requests iteratively
Hi Nithin,
Please see my answers inline.
Thanks,
Sorin
-Original Message-
From: Nithin Raju [mailto:nit...@vmware.com]
Sent: Thursday, 27 August, 2015 23:44
To: Sorin Vinturis
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v2 1/2] datapath-windows: Process tunnel filter
requests iteratively
> On Jul 20, 2015, at 12:00 PM, Sorin Vinturis
> wrote:
>
> In order to support IRP cancelling mechanism for pending IRPs, all
> tunnel filter requests, VXLAN create/delete tunnel, need to be
> processed iteratively.
>
> Signed-off-by: Sorin Vinturis
hi Sorin,
I had a question regarding latency of processing the requests. Looks good
otherwise. A v4 should be good to go.
> -do
> -{
> +do {
> if (!InterlockedCompareExchange(
> (LONG volatile *)&threadCtx->listRequests.numEntries, 0, 0)) {
> OVS_LOG_INFO("Nothing to do... request list is empty.");
> @@ -1076,38 +1056,25 @@
> OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
> }
> inTransaction = TRUE;
>
> -InitializeListHead(&head);
> -OvsTunnelFilterRequestPopList(&threadCtx->listRequests, &head,
> &count);
> -
> -LIST_FORALL_SAFE(&head, link, next) {
> -request = CONTAINING_RECORD(link, OVS_TUNFLT_REQUEST, entry);
> +while (NULL != (request = OvsTunnelFilterRequestPop(
> +&threadCtx->listRequests))) {
minor: indentation can be improved to:
while (NULL !=
(request = OvsTunnelFilterRequestPop(&threadCtx->listRequests))) {
SV: Sure.
>
> status = OvsTunnelFilterExecuteAction(threadCtx->engineSession,
> request);
> +
> +/* Complete the IRP with the last operation status. */
> +OvsTunnelFilterCompleteRequest(request, status);
> +
> +OvsFreeMemory(request);
> +request = NULL;
> +
> if (!NT_SUCCESS(status)) {
> -RemoveEntryList(&request->entry);
> -count--;
> -
> -/* Complete the IRP with the failure status. */
> -OvsTunnelFilterCompleteRequest(request->irp,
> - request->callback,
> - request->context,
> - status);
> -OvsFreeMemory(request);
> -request = NULL;
> -} else {
> -error = FALSE;
> +break;
> }
> }
>
What happens if there are 10 requests to process, and the 5th one fails. You’d
bail out of processing commit the 4 and go back into calling
KeWaitForMultipleObjects() in OvsTunnelFilterThreadProc(). What is the trigger
to processing the remaining 5 request, esp. if they are legitimate requests and
there’s a good chance that they’ll succeed.
SV: You are right. If there are more requests queued and an error happened to
one of them, the remaining request would not be processed until another one
will be received and the request event is signaled. I'll change the code to
continue with the processing the remaining requests, in case of error, instead
of breaking.
Also, if you are worried about the number of requests being committed per
transaction, maybe you should submit them in batches.
SV: No, I am not worried about the number of requests being too large for a
transaction to handle. I've tested by pushing 2000 of requests on 8 threads and
the code behaves fine. The threads number could be raised to handle an
increased number of requests if necessary.
Does KeSetEvent() queue up wakeup events?
SV: No, the KeSetEvent() does not queue up the events.
-- Nithin
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/2] datapath-windows: Process tunnel filter requests iteratively
hi Sorin,
Looks like you acknowledge my comments. I’ll look forward to the v3. Thanks for
your patience.
-- Nithin
> On Aug 31, 2015, at 5:08 AM, Sorin Vinturis
> wrote:
>
> Hi Nithin,
>
> Please see my answers inline.
>
> Thanks,
> Sorin
>
> -Original Message-
> From: Nithin Raju [mailto:nit...@vmware.com]
> Sent: Thursday, 27 August, 2015 23:44
> To: Sorin Vinturis
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v2 1/2] datapath-windows: Process tunnel filter
> requests iteratively
>
>> On Jul 20, 2015, at 12:00 PM, Sorin Vinturis
>> wrote:
>>
>> In order to support IRP cancelling mechanism for pending IRPs, all
>> tunnel filter requests, VXLAN create/delete tunnel, need to be
>> processed iteratively.
>>
>> Signed-off-by: Sorin Vinturis
>
> hi Sorin,
> I had a question regarding latency of processing the requests. Looks good
> otherwise. A v4 should be good to go.
>
>> -do
>> -{
>> +do {
>>if (!InterlockedCompareExchange(
>>(LONG volatile *)&threadCtx->listRequests.numEntries, 0, 0)) {
>>OVS_LOG_INFO("Nothing to do... request list is empty.");
>> @@ -1076,38 +1056,25 @@
>> OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
>>}
>>inTransaction = TRUE;
>>
>> -InitializeListHead(&head);
>> -OvsTunnelFilterRequestPopList(&threadCtx->listRequests, &head,
>> &count);
>> -
>> -LIST_FORALL_SAFE(&head, link, next) {
>> -request = CONTAINING_RECORD(link, OVS_TUNFLT_REQUEST, entry);
>> +while (NULL != (request = OvsTunnelFilterRequestPop(
>> +&threadCtx->listRequests))) {
>
> minor: indentation can be improved to:
> while (NULL !=
> (request = OvsTunnelFilterRequestPop(&threadCtx->listRequests)))
> {
> SV: Sure.
>
>>
>>status = OvsTunnelFilterExecuteAction(threadCtx->engineSession,
>> request);
>> +
>> +/* Complete the IRP with the last operation status. */
>> +OvsTunnelFilterCompleteRequest(request, status);
>> +
>> +OvsFreeMemory(request);
>> +request = NULL;
>> +
>>if (!NT_SUCCESS(status)) {
>> -RemoveEntryList(&request->entry);
>> -count--;
>> -
>> -/* Complete the IRP with the failure status. */
>> -OvsTunnelFilterCompleteRequest(request->irp,
>> - request->callback,
>> - request->context,
>> - status);
>> -OvsFreeMemory(request);
>> -request = NULL;
>> -} else {
>> -error = FALSE;
>> +break;
>>}
>>}
>>
>
> What happens if there are 10 requests to process, and the 5th one fails.
> You’d bail out of processing commit the 4 and go back into calling
> KeWaitForMultipleObjects() in OvsTunnelFilterThreadProc(). What is the
> trigger to processing the remaining 5 request, esp. if they are legitimate
> requests and there’s a good chance that they’ll succeed.
> SV: You are right. If there are more requests queued and an error happened to
> one of them, the remaining request would not be processed until another one
> will be received and the request event is signaled. I'll change the code to
> continue with the processing the remaining requests, in case of error,
> instead of breaking.
>
> Also, if you are worried about the number of requests being committed per
> transaction, maybe you should submit them in batches.
> SV: No, I am not worried about the number of requests being too large for a
> transaction to handle. I've tested by pushing 2000 of requests on 8 threads
> and the code behaves fine. The threads number could be raised to handle an
> increased number of requests if necessary.
>
> Does KeSetEvent() queue up wakeup events?
> SV: No, the KeSetEvent() does not queue up the events.
>
> -- Nithin
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ovsdb: Remove misleading OVS_UNUSED from ovsdb_monitor_change_cb().
This function does use this parameter.
(This does not change any behavior.)
Signed-off-by: Ben Pfaff
---
ovsdb/monitor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index 133460e..8a64fc1 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -779,7 +779,7 @@ ovsdb_monitor_changes_classify(enum ovsdb_monitor_selection
type,
static bool
ovsdb_monitor_change_cb(const struct ovsdb_row *old,
const struct ovsdb_row *new,
-const unsigned long int *changed OVS_UNUSED,
+const unsigned long int *changed,
void *aux_)
{
struct ovsdb_monitor_aux *aux = aux_;
--
2.1.3
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ovsdb: Update _version more accurately in transaction commit.
The _version column in each OVSDB row is supposed to be updated whenever
any other column in the row changes. However, the transaction code was
not careful to do this only when a row actually changed--there were other
cases where a row was considered at transaction commit time and _version
updated even though the row did not actually change. For example,
ovsdb_txn_adjust_atom_refs() calls find_or_make_txn_row(), which calls
ovsdb_txn_row_modify(), which updates _version, but
ovsdb_txn_adjust_atom_refs() doesn't actually update any data.
One way to fix this would be to carefully consider and adjust all the code
that looks at transaction rows. However, this seems somewhat error prone
and thus difficult to test. This commit takes a different approach: it
drops the code that adjusts _version on the fly, instead replacing it by
a final pass over the database at the end of the commit process that checks
for each row whether any columns changed and updates _version at that point
if any did. That seems pretty foolproof to me.
Reported-by: RishiRaj Maulick
Reported-at: http://openvswitch.org/pipermail/dev/2015-August/059439.html
Signed-off-by: Ben Pfaff
---
ovsdb/transaction.c | 25 ++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index 83ddaff..2c85fee 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -534,7 +534,6 @@ assess_weak_refs(struct ovsdb_txn *txn, struct
ovsdb_txn_row *txn_row)
}
if (datum->n != orig_n) {
-bitmap_set1(txn_row->changed, OVSDB_COL_VERSION);
bitmap_set1(txn_row->changed, column->index);
ovsdb_datum_sort_assert(datum, column->type.key.type);
if (datum->n < column->type.n_min) {
@@ -748,6 +747,21 @@ check_index_uniqueness(struct ovsdb_txn *txn OVS_UNUSED,
return NULL;
}
+static struct ovsdb_error * OVS_WARN_UNUSED_RESULT
+update_version(struct ovsdb_txn *txn OVS_UNUSED, struct ovsdb_txn_row *txn_row)
+{
+struct ovsdb_table *table = txn_row->table;
+size_t n_columns = shash_count(&table->schema->columns);
+
+if (txn_row->old && txn_row->new
+&& !bitmap_is_all_zeros(txn_row->changed, n_columns)) {
+bitmap_set1(txn_row->changed, OVSDB_COL_VERSION);
+uuid_generate(ovsdb_row_get_version_rw(txn_row->new));
+}
+
+return NULL;
+}
+
static struct ovsdb_error *
ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
{
@@ -801,6 +815,12 @@ ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
return error;
}
+/* Update _version for rows that changed. */
+error = for_each_txn_row(txn, update_version);
+if (error) {
+return OVSDB_WRAP_BUG("can't happen", error);
+}
+
/* Send the commit to each replica. */
LIST_FOR_EACH (replica, node, &txn->db->replicas) {
error = (replica->class->commit)(replica, txn, durable);
@@ -915,7 +935,6 @@ ovsdb_txn_row_modify(struct ovsdb_txn *txn, const struct
ovsdb_row *ro_row_)
rw_row = ovsdb_row_clone(ro_row);
rw_row->n_refs = ro_row->n_refs;
-uuid_generate(ovsdb_row_get_version_rw(rw_row));
ovsdb_txn_row_create(txn, table, ro_row, rw_row);
hmap_replace(&table->rows, &ro_row->hmap_node, &rw_row->hmap_node);
--
2.1.3
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Query with respect to OVSDB and vswitchd
On Mon, Aug 31, 2015 at 09:10:19AM +0530, RishiRaj Maulick wrote: > Hi , > > I have been very recently using the OVSDB for one of our projects . I am > able to create my own schema and operate on it . However to better > understand things , I was also looking at the behavior of vswitchd on the > default schema. > > I am confused with the events of things that happened - > > 1) I am monitoring the "Bridge" table through ovsdb-client > 2) Using ovs-vsctl , whenever I add a bridge ( say br0 ) , on the monitor > console I see that there are 2 activities - one of which is the initial > "insert" and the next is an update for the "datapath id " that gets > allocated by the vswitchd. > This is understandable. In the update the _version field and the datapath > id field gets mutated to new values. > 3) However, if I add a second bridge ( say br1 ) , in the monitor console , > apart from the above operations pertaining to the new bridge being added ( > br1 ) , I am also seeing that the entry for the previously added bridge br0 > also gets mutated and the only change is the _version field. > > My understanding of the _uuid and _version fields is that these are > maintained by the DB for internal purposes and the ovsdb-client cannot > mutate these. > > So I wanted to understand , why ( or what is the need ) the change of > *_version* for all of the existing bridges are required , whenever we are > adding a new bridge. It's just a bug. I sent out a fix. Will you test it? http://openvswitch.org/pipermail/dev/2015-August/059445.html Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 2/2] datapath-windows: Support for IRP cancelling mechanism
Hi Nithin,
Please see my answers inline.
-Original Message-
From: Nithin Raju [mailto:nit...@vmware.com]
Sent: Friday, 28 August, 2015 01:03
To: Sorin Vinturis
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v2 2/2] datapath-windows: Support for IRP
cancelling mechanism
hi Sorin,
Looks good overall.
I just one one comment about the patch which I’ve inlined.
How did you test this BTW?
SV: I've tested using instrumentation code.
> VOID
> @@ -1309,7 +1405,7 @@
> OvsTunnelFilterThreadUninit(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
> *
> --
>
> * This function creates a new tunnel filter request and push it to a
> thread
> * queue. If the thread stop event is signaled, the request is
> completed with
> - * STATUS_CANCELLED without pushing it to any queue.
> + * STATUS_REQUEST_ABORTED without pushing it to any queue.
> *
> --
>
> */
> NTSTATUS
> @@ -1321,7 +1417,7 @@ OvsTunnelFilterQueueRequest(PIRP irp,
> PVOID tunnelContext) {
> POVS_TUNFLT_REQUEST request = NULL;
> -NTSTATUSstatus = STATUS_PENDING;
> +NTSTATUSstatus = STATUS_SUCCESS;
> BOOLEAN error = TRUE;
> UINT64 timeout = 0;
>
> @@ -1334,8 +1430,8 @@ OvsTunnelFilterQueueRequest(PIRP irp,
> FALSE,
> (LARGE_INTEGER *)&timeout)) {
> /* The stop event is signaled. Completed the IRP with
> - * STATUS_CANCELLED. */
> -status = STATUS_CANCELLED;
> + * STATUS_REQUEST_ABORTED. */
> +status = STATUS_REQUEST_ABORTED;
> break;
> }
>
> @@ -1366,7 +1462,10 @@ OvsTunnelFilterQueueRequest(PIRP irp,
> request->callback = callback;
> request->context = tunnelContext;
>
> -OvsTunnelFilterThreadPush(request);
> +status = OvsTunnelFilterThreadPush(request);
> +if (!NT_SUCCESS(status)) {
> +break;
> +}
>
> error = FALSE;
> } while (error);
> @@ -1379,13 +1478,14 @@ OvsTunnelFilterQueueRequest(PIRP irp,
> }
> }
>
> -return status;
> +/* Return pending to indicate that the IRP will be completed later. */
> +return STATUS_PENDING;
You are returning STATUS_PENDING at the end of the function. You should be
returning ‘status’.
SV: Correct. Good catch.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v3 1/2] datapath-windows: Process tunnel filter requests iteratively
In order to support IRP cancelling mechanism for pending IRPs, all
tunnel filter requests, VXLAN create/delete tunnel, need to be
processed iteratively.
Signed-off-by: Sorin Vinturis
---
datapath-windows/ovsext/TunnelFilter.c | 103 ++---
1 file changed, 30 insertions(+), 73 deletions(-)
diff --git a/datapath-windows/ovsext/TunnelFilter.c
b/datapath-windows/ovsext/TunnelFilter.c
index 00e57c3..6cf9a94 100644
--- a/datapath-windows/ovsext/TunnelFilter.c
+++ b/datapath-windows/ovsext/TunnelFilter.c
@@ -955,38 +955,25 @@ OvsTunnelFilterExecuteAction(HANDLE engineSession,
/*
* --
- * This function pops the whole request entries from the queue and returns the
- * number of entries through the 'count' parameter. The operation is
- * synchronized using request list spinlock.
+ * This function pops the head item from the requests list while holding
+ * the list's spinlock.
* --
*/
-VOID
-OvsTunnelFilterRequestPopList(POVS_TUNFLT_REQUEST_LIST listRequests,
- PLIST_ENTRY head,
- UINT32 *count)
+POVS_TUNFLT_REQUEST
+OvsTunnelFilterRequestPop(POVS_TUNFLT_REQUEST_LIST listRequests)
{
+POVS_TUNFLT_REQUEST request = NULL;
+
NdisAcquireSpinLock(&listRequests->spinlock);
if (!IsListEmpty(&listRequests->head)) {
-PLIST_ENTRY PrevEntry;
-PLIST_ENTRY NextEntry;
-
-NextEntry = listRequests->head.Flink;
-PrevEntry = listRequests->head.Blink;
-
-head->Flink = NextEntry;
-NextEntry->Blink = head;
-
-head->Blink = PrevEntry;
-PrevEntry->Flink = head;
-
-*count = listRequests->numEntries;
-
-InitializeListHead(&listRequests->head);
-listRequests->numEntries = 0;
+request = (POVS_TUNFLT_REQUEST)RemoveHeadList(&listRequests->head);
+listRequests->numEntries--;
}
NdisReleaseSpinLock(&listRequests->spinlock);
+
+return request;
}
/*
@@ -1056,16 +1043,10 @@ VOID
OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
{
POVS_TUNFLT_REQUEST request = NULL;
-PLIST_ENTRY link = NULL;
-PLIST_ENTRY next = NULL;
-LIST_ENTRY head;
NTSTATUSstatus = STATUS_SUCCESS;
-UINT32 count = 0;
BOOLEAN inTransaction = FALSE;
-BOOLEAN error = TRUE;
-do
-{
+do {
if (!InterlockedCompareExchange(
(LONG volatile *)&threadCtx->listRequests.numEntries, 0, 0)) {
OVS_LOG_INFO("Nothing to do... request list is empty.");
@@ -1080,38 +1061,24 @@
OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
}
inTransaction = TRUE;
-InitializeListHead(&head);
-OvsTunnelFilterRequestPopList(&threadCtx->listRequests, &head, &count);
-
-LIST_FORALL_SAFE(&head, link, next) {
-request = CONTAINING_RECORD(link, OVS_TUNFLT_REQUEST, entry);
-
-status = OvsTunnelFilterExecuteAction(threadCtx->engineSession,
- request);
-if (!NT_SUCCESS(status)) {
-RemoveEntryList(&request->entry);
-count--;
-
-/* Complete the IRP with the failure status. */
-OvsTunnelFilterCompleteRequest(request->irp,
- request->callback,
- request->context,
- status);
-OvsFreeMemory(request);
-request = NULL;
-} else {
-error = FALSE;
-}
-}
+while (NULL !=
+(request = OvsTunnelFilterRequestPop(&threadCtx->listRequests))) {
-if (error) {
-/* No successful requests were made, so there is no point to commit
- * the transaction. */
-break;
+OvsTunnelFilterExecuteAction(threadCtx->engineSession,
+ request);
+
+/* Complete the IRP with the last operation status. */
+OvsTunnelFilterCompleteRequest(request->irp,
+ request->callback,
+ request->context,
+ status);
+
+OvsFreeMemory(request);
+request = NULL;
}
status = FwpmTransactionCommit(threadCtx->engineSession);
-if (!NT_SUCCESS(status)){
+if (!NT_SUCCESS(status)) {
OVS_LOG_ERROR("Failed to commit transaction, status: %x.",
status);
break;
@@ -1125,20 +1092,6 @@
OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threa
[ovs-dev] [PATCH v3 0/2] datapath-windows: IRP cancelling mechanism
This patch series adds support for IRP cancelling mechanism to the OVS extension. Sorin Vinturis (2): [PATCH v3 1/2] datapath-windows: Process tunnel filter requests iteratively [PATCH v3 2/2] datapath-windows: Support for IRP cancelling mechanism ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v3 2/2] datapath-windows: Support for IRP cancelling mechanism
Under certain circumstances, we might need to cancel a pending IRP
that has been submitted and not yet responded. This might occur when
the request takes too long to complete or when the process which
initiated the request terminated, leaving the request outstanding.
This patch provides this missing piece by adding support for IRP
cancelling mechanism.
Signed-off-by: Sorin Vinturis
Reported-by: Sorin Vinturis
Reported-at: https://github.com/openvswitch/ovs-issues/issues/95
v2: Before pending an IRP, check if it has been cancelled.
---
datapath-windows/ovsext/Datapath.c | 4 -
datapath-windows/ovsext/TunnelFilter.c | 198 ++---
2 files changed, 180 insertions(+), 22 deletions(-)
diff --git a/datapath-windows/ovsext/Datapath.c
b/datapath-windows/ovsext/Datapath.c
index 8c72533..b7bbf80 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -918,10 +918,6 @@ done:
exit:
/* Should not complete a pending IRP unless proceesing is completed. */
if (status == STATUS_PENDING) {
-/* STATUS_PENDING is returned by the NL handler when the request is
- * to be processed later, so we mark the IRP as pending and complete
- * it in another thread when the request is processed. */
-IoMarkIrpPending(irp);
return status;
}
return OvsCompleteIrpRequest(irp, (ULONG_PTR)replyLen, status);
diff --git a/datapath-windows/ovsext/TunnelFilter.c
b/datapath-windows/ovsext/TunnelFilter.c
index 6cf9a94..ec14c64 100644
--- a/datapath-windows/ovsext/TunnelFilter.c
+++ b/datapath-windows/ovsext/TunnelFilter.c
@@ -168,6 +168,10 @@ static VOID
OvsTunnelFilterThreadStop(POVS_TUNFLT_THREAD_CONTEXT threadCtx,
BOOLEAN signalEvent);
static NTSTATUS OvsTunnelFilterThreadInit(POVS_TUNFLT_THREAD_CONTEXT
threadCtx);
static VOID OvsTunnelFilterThreadUninit(POVS_TUNFLT_THREAD_CONTEXT
threadCtx);
+static VOID OvsTunnelFilterSetIrpContext(POVS_TUNFLT_REQUEST_LIST
listRequests,
+ POVS_TUNFLT_REQUEST request);
+static VOID OvsTunnelFilterCancelIrp(PDEVICE_OBJECT DeviceObject,
+ PIRP Irp);
/*
* Callout driver global variables
@@ -955,20 +959,49 @@ OvsTunnelFilterExecuteAction(HANDLE engineSession,
/*
* --
- * This function pops the head item from the requests list while holding
- * the list's spinlock.
+ * This function pops the head request from the queue while holding the
+ * queue lock. If the request has already been cancelled or is about to be
+ * cancelled, the function retrieves the next valid request.
+ *
+ * Returns a pointer to the OVS_TUNFLT_REQUEST_LIST request object retrieved
+ * from the queue.
* --
*/
POVS_TUNFLT_REQUEST
OvsTunnelFilterRequestPop(POVS_TUNFLT_REQUEST_LIST listRequests)
{
POVS_TUNFLT_REQUEST request = NULL;
+PLIST_ENTRY link, next, head;
NdisAcquireSpinLock(&listRequests->spinlock);
if (!IsListEmpty(&listRequests->head)) {
-request = (POVS_TUNFLT_REQUEST)RemoveHeadList(&listRequests->head);
-listRequests->numEntries--;
+head = &listRequests->head;
+LIST_FORALL_SAFE(head, link, next) {
+PDRIVER_CANCEL oldCancelRoutine;
+
+request = CONTAINING_RECORD(link, OVS_TUNFLT_REQUEST, entry);
+if (request->irp) {
+oldCancelRoutine = IoSetCancelRoutine(request->irp, NULL);
+if (oldCancelRoutine == NULL) {
+/*
+ * The Cancel routine for the current IRP is running. The
+ * request is to be completed by the Cancel routine. Leave
+ * this request alone and go to the next one.
+ */
+continue;
+} else {
+/*
+ * The Cancel routine cannot run now and cannot already
have
+ * started to run. This request can be processed.
+ */
+}
+}
+
+RemoveEntryList(&request->entry);
+listRequests->numEntries--;
+break;
+}
}
NdisReleaseSpinLock(&listRequests->spinlock);
@@ -978,20 +1011,78 @@ OvsTunnelFilterRequestPop(POVS_TUNFLT_REQUEST_LIST
listRequests)
/*
* --
- * This function pushes the received request to the list while holding the
- * request list spinlock.
+ * This function pushes the received request to the queue, marks the IRP as
+ * pending and sets its Cancel routine, while holding the queue lock.
+ *
+ * Returns STATUS_CANCELLED if the IRP has already been cancelled. Otherwise,
+ * STATUS_SUCCESS is returne
Re: [ovs-dev] [PATCH] flow: Fix MSVC compile errors.
> On Aug 30, 2015, at 7:40 AM, Ben Pfaff wrote: > > MSVC doesn't like the change in 'const' between function declaration and > definition: it reports "formal parameter 2 different from declaration" for > each of the functions in flow.h corrected by this (commit. I think it's > technically wrong about that, standards-wise.) > > MSVC doesn't like an empty-brace initializer. (I think it's technically > right about that, standards-wise.) > > This commit attempts to fix both problems, but I have not tested it with > MSVC. > > CC: Jarno Rajahalme > Signed-off-by: Ben Pfaff Thanks Ben for the patch. There seems to be one a change required in netdev-windows.c. I’ll send out a patch for that. Acked-by: Nithin Raju Tested-by: Nithin Raju thanks, -- Nithin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] lib/netdev-windows.c: typo in call to memcpy()
Signed-off-by: Nithin Raju --- lib/netdev-windows.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/netdev-windows.c b/lib/netdev-windows.c index 51d088a..aef9d2c 100644 --- a/lib/netdev-windows.c +++ b/lib/netdev-windows.c @@ -248,7 +248,7 @@ netdev_windows_netdev_from_ofpbuf(struct netdev_windows_netdev_info *info, info->port_no = nl_attr_get_odp_port(a[OVS_WIN_NETDEV_ATTR_PORT_NO]); info->ovs_type = nl_attr_get_u32(a[OVS_WIN_NETDEV_ATTR_TYPE]); info->name = nl_attr_get_string(a[OVS_WIN_NETDEV_ATTR_NAME]); -memcpy(info->mac_address, nl_attr_get_unspec(a[OVS_WIN_NETDEV_ATTR_MAC_ADDR], +memcpy(&info->mac_address, nl_attr_get_unspec(a[OVS_WIN_NETDEV_ATTR_MAC_ADDR], sizeof(info->mac_address)), sizeof(info->mac_address)); info->mtu = nl_attr_get_u32(a[OVS_WIN_NETDEV_ATTR_MTU]); info->ifi_flags = nl_attr_get_u32(a[OVS_WIN_NETDEV_ATTR_IF_FLAGS]); -- 1.9.4.msysgit.2 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3 1/2] datapath-windows: Process tunnel filter requests iteratively
> +while (NULL !=
> +(request = OvsTunnelFilterRequestPop(&threadCtx->listRequests)))
> {
>
> -if (error) {
> -/* No successful requests were made, so there is no point to
> commit
> - * the transaction. */
> -break;
> +OvsTunnelFilterExecuteAction(threadCtx->engineSession,
> + request);
Don’t you need a:
status = ?
> +
> +/* Complete the IRP with the last operation status. */
> +OvsTunnelFilterCompleteRequest(request->irp,
> + request->callback,
> + request->context,
> + status);
-- Nithin
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] datapath: check for rx handler register
On Fri, Aug 28, 2015 at 5:52 PM, Flavio Leitner wrote: > Red Hat Enterprise Linux 6 has backported the netdev RX > handler facility so use the netdev_rx_handler_register as > an indicator. > > The handler prototype changed between 2.6.36 and 2.6.39 > since there could be backports in any stage, don't look > at the kernel version, but at the prototype. > > Signed-off-by: Flavio Leitner Thanks, applied to master and branch-2.4. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] flow: Fix MSVC compile errors.
On Mon, Aug 31, 2015 at 05:56:34PM +, Nithin Raju wrote: > > > On Aug 30, 2015, at 7:40 AM, Ben Pfaff wrote: > > > > MSVC doesn't like the change in 'const' between function declaration and > > definition: it reports "formal parameter 2 different from declaration" for > > each of the functions in flow.h corrected by this (commit. I think it's > > technically wrong about that, standards-wise.) > > > > MSVC doesn't like an empty-brace initializer. (I think it's technically > > right about that, standards-wise.) > > > > This commit attempts to fix both problems, but I have not tested it with > > MSVC. > > > > CC: Jarno Rajahalme > > Signed-off-by: Ben Pfaff > > Thanks Ben for the patch. > > There seems to be one a change required in netdev-windows.c. I’ll send out a > patch for that. > > Acked-by: Nithin Raju > Tested-by: Nithin Raju Thanks, I've applied this to master. I'll look for the netdev-windows.c patch. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] lib/netdev-windows.c: typo in call to memcpy()
On Mon, Aug 31, 2015 at 10:59:28AM -0700, Nithin Raju wrote: > Signed-off-by: Nithin Raju Thanks, applied. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v4 1/2] datapath-windows: Process tunnel filter requests iteratively
In order to support IRP cancelling mechanism for pending IRPs, all
tunnel filter requests, VXLAN create/delete tunnel, need to be
processed iteratively.
Signed-off-by: Sorin Vinturis
---
datapath-windows/ovsext/TunnelFilter.c | 103 ++---
1 file changed, 30 insertions(+), 73 deletions(-)
diff --git a/datapath-windows/ovsext/TunnelFilter.c
b/datapath-windows/ovsext/TunnelFilter.c
index 00e57c3..6cf9a94 100644
--- a/datapath-windows/ovsext/TunnelFilter.c
+++ b/datapath-windows/ovsext/TunnelFilter.c
@@ -955,38 +955,25 @@ OvsTunnelFilterExecuteAction(HANDLE engineSession,
/*
* --
- * This function pops the whole request entries from the queue and returns the
- * number of entries through the 'count' parameter. The operation is
- * synchronized using request list spinlock.
+ * This function pops the head item from the requests list while holding
+ * the list's spinlock.
* --
*/
-VOID
-OvsTunnelFilterRequestPopList(POVS_TUNFLT_REQUEST_LIST listRequests,
- PLIST_ENTRY head,
- UINT32 *count)
+POVS_TUNFLT_REQUEST
+OvsTunnelFilterRequestPop(POVS_TUNFLT_REQUEST_LIST listRequests)
{
+POVS_TUNFLT_REQUEST request = NULL;
+
NdisAcquireSpinLock(&listRequests->spinlock);
if (!IsListEmpty(&listRequests->head)) {
-PLIST_ENTRY PrevEntry;
-PLIST_ENTRY NextEntry;
-
-NextEntry = listRequests->head.Flink;
-PrevEntry = listRequests->head.Blink;
-
-head->Flink = NextEntry;
-NextEntry->Blink = head;
-
-head->Blink = PrevEntry;
-PrevEntry->Flink = head;
-
-*count = listRequests->numEntries;
-
-InitializeListHead(&listRequests->head);
-listRequests->numEntries = 0;
+request = (POVS_TUNFLT_REQUEST)RemoveHeadList(&listRequests->head);
+listRequests->numEntries--;
}
NdisReleaseSpinLock(&listRequests->spinlock);
+
+return request;
}
/*
@@ -1056,16 +1043,10 @@ VOID
OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
{
POVS_TUNFLT_REQUEST request = NULL;
-PLIST_ENTRY link = NULL;
-PLIST_ENTRY next = NULL;
-LIST_ENTRY head;
NTSTATUSstatus = STATUS_SUCCESS;
-UINT32 count = 0;
BOOLEAN inTransaction = FALSE;
-BOOLEAN error = TRUE;
-do
-{
+do {
if (!InterlockedCompareExchange(
(LONG volatile *)&threadCtx->listRequests.numEntries, 0, 0)) {
OVS_LOG_INFO("Nothing to do... request list is empty.");
@@ -1080,38 +1061,24 @@
OvsTunnelFilterRequestListProcess(POVS_TUNFLT_THREAD_CONTEXT threadCtx)
}
inTransaction = TRUE;
-InitializeListHead(&head);
-OvsTunnelFilterRequestPopList(&threadCtx->listRequests, &head, &count);
-
-LIST_FORALL_SAFE(&head, link, next) {
-request = CONTAINING_RECORD(link, OVS_TUNFLT_REQUEST, entry);
-
-status = OvsTunnelFilterExecuteAction(threadCtx->engineSession,
- request);
-if (!NT_SUCCESS(status)) {
-RemoveEntryList(&request->entry);
-count--;
-
-/* Complete the IRP with the failure status. */
-OvsTunnelFilterCompleteRequest(request->irp,
- request->callback,
- request->context,
- status);
-OvsFreeMemory(request);
-request = NULL;
-} else {
-error = FALSE;
-}
-}
+while (NULL !=
+(request = OvsTunnelFilterRequestPop(&threadCtx->listRequests))) {
-if (error) {
-/* No successful requests were made, so there is no point to commit
- * the transaction. */
-break;
+status = OvsTunnelFilterExecuteAction(threadCtx->engineSession,
+ request);
+
+/* Complete the IRP with the last operation status. */
+OvsTunnelFilterCompleteRequest(request->irp,
+ request->callback,
+ request->context,
+ status);
+
+OvsFreeMemory(request);
+request = NULL;
}
status = FwpmTransactionCommit(threadCtx->engineSession);
-if (!NT_SUCCESS(status)){
+if (!NT_SUCCESS(status)) {
OVS_LOG_ERROR("Failed to commit transaction, status: %x.",
status);
break;
@@ -1125,20 +1092,6 @@
OvsTunnelFilterRequestListProcess(POVS_TUNFLT_TH
Re: [ovs-dev] [PATCH v4 1/2] datapath-windows: Process tunnel filter requests iteratively
> On Aug 31, 2015, at 1:53 PM, Sorin Vinturis > wrote: > > In order to support IRP cancelling mechanism for pending IRPs, all > tunnel filter requests, VXLAN create/delete tunnel, need to be > processed iteratively. > > Signed-off-by: Sorin Vinturis Acked-by: Nithin Raju ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3 2/2] datapath-windows: Support for IRP cancelling mechanism
> On Aug 31, 2015, at 10:46 AM, Sorin Vinturis > wrote: > > Under certain circumstances, we might need to cancel a pending IRP > that has been submitted and not yet responded. This might occur when > the request takes too long to complete or when the process which > initiated the request terminated, leaving the request outstanding. > > This patch provides this missing piece by adding support for IRP > cancelling mechanism. > > Signed-off-by: Sorin Vinturis > Reported-by: Sorin Vinturis Acked-by: Nithin Raju ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 1/2] datapath-windows: Process tunnel filter requests iteratively
On Mon, Aug 31, 2015 at 09:11:05PM +, Nithin Raju wrote: > > On Aug 31, 2015, at 1:53 PM, Sorin Vinturis > > wrote: > > > > In order to support IRP cancelling mechanism for pending IRPs, all > > tunnel filter requests, VXLAN create/delete tunnel, need to be > > processed iteratively. > > > > Signed-off-by: Sorin Vinturis > > Acked-by: Nithin Raju Thanks Sorin and Nithin, applied to master and branch-2.4. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v3 2/2] datapath-windows: Support for IRP cancelling mechanism
On Mon, Aug 31, 2015 at 09:12:18PM +, Nithin Raju wrote: > > On Aug 31, 2015, at 10:46 AM, Sorin Vinturis > > wrote: > > > > Under certain circumstances, we might need to cancel a pending IRP > > that has been submitted and not yet responded. This might occur when > > the request takes too long to complete or when the process which > > initiated the request terminated, leaving the request outstanding. > > > > This patch provides this missing piece by adding support for IRP > > cancelling mechanism. > > > > Signed-off-by: Sorin Vinturis > > Reported-by: Sorin Vinturis > > Acked-by: Nithin Raju Thanks Sorin and Nithin, applied to master and branch-2.4. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovsdb: Remove misleading OVS_UNUSED from ovsdb_monitor_change_cb().
Acked-by: Andy Zhou
On Mon, Aug 31, 2015 at 9:44 AM, Ben Pfaff wrote:
> This function does use this parameter.
>
> (This does not change any behavior.)
>
> Signed-off-by: Ben Pfaff
> ---
> ovsdb/monitor.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> index 133460e..8a64fc1 100644
> --- a/ovsdb/monitor.c
> +++ b/ovsdb/monitor.c
> @@ -779,7 +779,7 @@ ovsdb_monitor_changes_classify(enum
> ovsdb_monitor_selection type,
> static bool
> ovsdb_monitor_change_cb(const struct ovsdb_row *old,
> const struct ovsdb_row *new,
> -const unsigned long int *changed OVS_UNUSED,
> +const unsigned long int *changed,
> void *aux_)
> {
> struct ovsdb_monitor_aux *aux = aux_;
> --
> 2.1.3
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovsdb: Remove misleading OVS_UNUSED from ovsdb_monitor_change_cb().
Thanks, applied.
On Mon, Aug 31, 2015 at 02:36:28PM -0700, Andy Zhou wrote:
> Acked-by: Andy Zhou
>
> On Mon, Aug 31, 2015 at 9:44 AM, Ben Pfaff wrote:
> > This function does use this parameter.
> >
> > (This does not change any behavior.)
> >
> > Signed-off-by: Ben Pfaff
> > ---
> > ovsdb/monitor.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> > index 133460e..8a64fc1 100644
> > --- a/ovsdb/monitor.c
> > +++ b/ovsdb/monitor.c
> > @@ -779,7 +779,7 @@ ovsdb_monitor_changes_classify(enum
> > ovsdb_monitor_selection type,
> > static bool
> > ovsdb_monitor_change_cb(const struct ovsdb_row *old,
> > const struct ovsdb_row *new,
> > -const unsigned long int *changed OVS_UNUSED,
> > +const unsigned long int *changed,
> > void *aux_)
> > {
> > struct ovsdb_monitor_aux *aux = aux_;
> > --
> > 2.1.3
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Dev@openvswitch.org
Dear user of openvswitch.org, We have found that your account has been used to send a huge amount of spam messages during the last week. Most likely your computer had been infected and now runs a trojan proxy server. Please follow instruction in order to keep your computer safe. Best wishes, The openvswitch.org support team. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/3] ofp-parse: Allow ofctl flow monitor filtering on field existence.
It is supposed to be possible to allow ovs-ofctl to filter flows
it is monitoring based on a match string. However, the parser will
reject expressions that match only on a field's existence (such as
Geneve options). This relaxes the restriction to bring it in line
with matches supported by other commands.
Signed-off-by: Jesse Gross
---
lib/ofp-parse.c | 70 +
1 file changed, 36 insertions(+), 34 deletions(-)
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index a6190ed..855d732 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -221,6 +221,12 @@ parse_field(const struct mf_field *mf, const char *s,
struct match *match,
union mf_value value, mask;
char *error;
+if (!s) {
+/* If there's no string, we're just trying to match on the
+ * existence of the field, so use a no-op value. */
+s = "0/0";
+}
+
error = mf_parse(mf, s, &value, &mask);
if (!error) {
*usable_protocols &= mf_set(mf, &value, &mask, match);
@@ -372,11 +378,6 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command,
char *string,
value = field;
if (mf_from_name(name)) {
-if (!value) {
-/* If there's no value, we're just trying to match on the
- * existence of the field, so use a no-op value. */
- value = "0/0";
-}
error = parse_field(mf_from_name(name), value, &fm->match,
usable_protocols);
} else {
@@ -774,7 +775,7 @@ parse_flow_monitor_request__(struct
ofputil_flow_monitor_request *fmr,
{
static atomic_count id = ATOMIC_COUNT_INIT(0);
char *save_ptr = NULL;
-char *name;
+char *field;
fmr->id = atomic_count_inc(&id);
@@ -784,52 +785,53 @@ parse_flow_monitor_request__(struct
ofputil_flow_monitor_request *fmr,
fmr->table_id = 0xff;
match_init_catchall(&fmr->match);
-for (name = strtok_r(string, "=, \t\r\n", &save_ptr); name;
- name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) {
+for (field = strtok_r(string, ", \t\r\n", &save_ptr); field;
+ field = strtok_r(NULL, ", \t\r\n", &save_ptr)) {
const struct protocol *p;
-if (!strcmp(name, "!initial")) {
+if (!strcmp(field, "!initial")) {
fmr->flags &= ~NXFMF_INITIAL;
-} else if (!strcmp(name, "!add")) {
+} else if (!strcmp(field, "!add")) {
fmr->flags &= ~NXFMF_ADD;
-} else if (!strcmp(name, "!delete")) {
+} else if (!strcmp(field, "!delete")) {
fmr->flags &= ~NXFMF_DELETE;
-} else if (!strcmp(name, "!modify")) {
+} else if (!strcmp(field, "!modify")) {
fmr->flags &= ~NXFMF_MODIFY;
-} else if (!strcmp(name, "!actions")) {
+} else if (!strcmp(field, "!actions")) {
fmr->flags &= ~NXFMF_ACTIONS;
-} else if (!strcmp(name, "!own")) {
+} else if (!strcmp(field, "!own")) {
fmr->flags &= ~NXFMF_OWN;
-} else if (parse_protocol(name, &p)) {
+} else if (parse_protocol(field, &p)) {
match_set_dl_type(&fmr->match, htons(p->dl_type));
if (p->nw_proto) {
match_set_nw_proto(&fmr->match, p->nw_proto);
}
} else {
-char *value;
-
-value = strtok_r(NULL, ", \t\r\n", &save_ptr);
-if (!value) {
-return xasprintf("%s: field %s missing value", str_, name);
-}
+char *name, *value;
+char *error = NULL;
-if (!strcmp(name, "table")) {
-char *error = str_to_u8(value, "table", &fmr->table_id);
-if (error) {
-return error;
-}
-} else if (!strcmp(name, "out_port")) {
-fmr->out_port = u16_to_ofp(atoi(value));
-} else if (mf_from_name(name)) {
-char *error;
+name = strsep(&field, "=");
+value = field;
+if (mf_from_name(name)) {
error = parse_field(mf_from_name(name), value, &fmr->match,
usable_protocols);
-if (error) {
-return error;
-}
} else {
-return xasprintf("%s: unknown keyword %s", str_, name);
+if (!value) {
+return xasprintf("%s: field %s missing value", str_, name);
+}
+
+if (!strcmp(name, "table")) {
+error = str_to_u8(value, "table", &fmr->table_id);
+} else if (!strcmp(name, "out_port")) {
+fmr->out_port = u16_to_ofp(atoi(value));
+} else {
+return xasprintf("%s: unknown keyword %s", str_, name);
+}
+}
+
+
[ovs-dev] [PATCH 1/3] ofp-parse: Separate fields properly.
Currently, each token in an OpenFlow match field is treated separately -
whether this is a name, a value, or a single identifier. However, this
means that attempting to get a value may result in grabbing the next
token if no value exists. This avoids that problem by breaking the match
string down into its components and then individually separating it into
name/value pairs if appropriate.
Signed-off-by: Jesse Gross
---
lib/ofp-parse.c | 165
1 file changed, 81 insertions(+), 84 deletions(-)
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index eaaa8ba..a6190ed 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -256,7 +256,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command,
char *string,
} fields;
char *save_ptr = NULL;
char *act_str = NULL;
-char *name;
+char *field;
*usable_protocols = OFPUTIL_P_ANY;
@@ -339,116 +339,113 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int
command, char *string,
return xstrdup("must specify an action");
}
}
-for (name = strtok_r(string, "=, \t\r\n", &save_ptr); name;
- name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) {
+for (field = strtok_r(string, ", \t\r\n", &save_ptr); field;
+ field = strtok_r(NULL, ", \t\r\n", &save_ptr)) {
const struct protocol *p;
char *error = NULL;
-if (parse_protocol(name, &p)) {
+if (parse_protocol(field, &p)) {
match_set_dl_type(&fm->match, htons(p->dl_type));
if (p->nw_proto) {
match_set_nw_proto(&fm->match, p->nw_proto);
}
-} else if (fields & F_FLAGS && !strcmp(name, "send_flow_rem")) {
+} else if (fields & F_FLAGS && !strcmp(field, "send_flow_rem")) {
fm->flags |= OFPUTIL_FF_SEND_FLOW_REM;
-} else if (fields & F_FLAGS && !strcmp(name, "check_overlap")) {
+} else if (fields & F_FLAGS && !strcmp(field, "check_overlap")) {
fm->flags |= OFPUTIL_FF_CHECK_OVERLAP;
-} else if (fields & F_FLAGS && !strcmp(name, "reset_counts")) {
+} else if (fields & F_FLAGS && !strcmp(field, "reset_counts")) {
fm->flags |= OFPUTIL_FF_RESET_COUNTS;
*usable_protocols &= OFPUTIL_P_OF12_UP;
-} else if (fields & F_FLAGS && !strcmp(name, "no_packet_counts")) {
+} else if (fields & F_FLAGS && !strcmp(field, "no_packet_counts")) {
fm->flags |= OFPUTIL_FF_NO_PKT_COUNTS;
*usable_protocols &= OFPUTIL_P_OF13_UP;
-} else if (fields & F_FLAGS && !strcmp(name, "no_byte_counts")) {
+} else if (fields & F_FLAGS && !strcmp(field, "no_byte_counts")) {
fm->flags |= OFPUTIL_FF_NO_BYT_COUNTS;
*usable_protocols &= OFPUTIL_P_OF13_UP;
-} else if (!strcmp(name, "no_readonly_table")
- || !strcmp(name, "allow_hidden_fields")) {
+} else if (!strcmp(field, "no_readonly_table")
+ || !strcmp(field, "allow_hidden_fields")) {
/* ignore these fields. */
-} else if (mf_from_name(name)) {
-char *value;
-
-value = strtok_r(NULL, ", \t\r\n", &save_ptr);
-if (!value) {
-/* If there's no value, we're just trying to match on the
- * existence of the field, so use a no-op value. */
-value = "0/0";
-}
-
-error = parse_field(mf_from_name(name), value, &fm->match,
-usable_protocols);
-if (error) {
-return error;
-}
} else {
-char *value;
+char *name, *value;
-value = strtok_r(NULL, ", \t\r\n", &save_ptr);
-if (!value) {
-return xasprintf("field %s missing value", name);
-}
+name = strsep(&field, "=");
+value = field;
-if (!strcmp(name, "table")) {
-error = str_to_u8(value, "table", &fm->table_id);
-if (fm->table_id != 0xff) {
-*usable_protocols &= OFPUTIL_P_TID;
+if (mf_from_name(name)) {
+if (!value) {
+/* If there's no value, we're just trying to match on the
+ * existence of the field, so use a no-op value. */
+ value = "0/0";
}
-} else if (fields & F_OUT_PORT && !strcmp(name, "out_port")) {
-if (!ofputil_port_from_string(value, &fm->out_port)) {
-error = xasprintf("%s is not a valid OpenFlow port",
- value);
+error = parse_field(mf_from_name(name), value, &fm->match,
+usable_protocols);
+} else {
+if (!value) {
+return xasprintf("field %s missing value", name);
[ovs-dev] [PATCH 3/3] tun-metadata: Provide error messages during auto-allocation.
In cases where we don't have a map of tunnel metadata options (such
as with ovs-ofctl) we dynamically allocate them as part of the match.
However, dynamic allocation brings the possibility of errors such as
duplicate entries or running out of space. Up until now, anything that
would cause an error was silently ignored. Since that is not very user
friendly, this adds a mechanism for reporting these types of errors.
Signed-off-by: Jesse Gross
---
lib/meta-flow.c| 52 ++--
lib/meta-flow.h| 6 +++---
lib/nx-match.c | 10 +-
lib/ofp-parse.c| 2 +-
lib/tun-metadata.c | 35 ---
lib/tun-metadata.h | 9 +
6 files changed, 84 insertions(+), 30 deletions(-)
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index e2e61f8..224ba53 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -796,11 +796,19 @@ mf_get_value(const struct mf_field *mf, const struct flow
*flow,
/* Makes 'match' match field 'mf' exactly, with the value matched taken from
* 'value'. The caller is responsible for ensuring that 'match' meets 'mf''s
- * prerequisites. */
+ * prerequisites.
+ *
+ * If non-NULL, 'err_str' returns a malloc'ed string describing any errors
+ * with the request or NULL if there is no error. The caller is reponsible
+ * for freeing the string. */
void
mf_set_value(const struct mf_field *mf,
- const union mf_value *value, struct match *match)
+ const union mf_value *value, struct match *match, char **err_str)
{
+if (err_str) {
+*err_str = NULL;
+}
+
switch (mf->id) {
case MFF_DP_HASH:
match_set_dp_hash(match, ntohl(value->be32));
@@ -836,7 +844,7 @@ mf_set_value(const struct mf_field *mf,
match_set_tun_ttl(match, value->u8);
break;
CASE_MFF_TUN_METADATA:
-tun_metadata_set_match(mf, value, NULL, match);
+tun_metadata_set_match(mf, value, NULL, match, err_str);
break;
case MFF_METADATA:
@@ -1364,10 +1372,18 @@ mf_is_set(const struct mf_field *mf, const struct flow
*flow)
/* Makes 'match' wildcard field 'mf'.
*
* The caller is responsible for ensuring that 'match' meets 'mf''s
- * prerequisites. */
+ * prerequisites.
+ *
+ * If non-NULL, 'err_str' returns a malloc'ed string describing any errors
+ * with the request or NULL if there is no error. The caller is reponsible
+ * for freeing the string. */
void
-mf_set_wild(const struct mf_field *mf, struct match *match)
+mf_set_wild(const struct mf_field *mf, struct match *match, char **err_str)
{
+if (err_str) {
+*err_str = NULL;
+}
+
switch (mf->id) {
case MFF_DP_HASH:
match->flow.dp_hash = 0;
@@ -1406,7 +1422,7 @@ mf_set_wild(const struct mf_field *mf, struct match
*match)
match_set_tun_ttl_masked(match, 0, 0);
break;
CASE_MFF_TUN_METADATA:
-tun_metadata_set_match(mf, NULL, NULL, match);
+tun_metadata_set_match(mf, NULL, NULL, match, err_str);
break;
case MFF_METADATA:
@@ -1595,22 +1611,30 @@ mf_set_wild(const struct mf_field *mf, struct match
*match)
* call is equivalent to mf_set_wild(mf, match).
*
* 'mask' must be a valid mask for 'mf' (see mf_is_mask_valid()). The caller
- * is responsible for ensuring that 'match' meets 'mf''s prerequisites. */
+ * is responsible for ensuring that 'match' meets 'mf''s prerequisites.
+ *
+ * If non-NULL, 'err_str' returns a malloc'ed string describing any errors
+ * with the request or NULL if there is no error. The caller is reponsible
+ * for freeing the string.*/
enum ofputil_protocol
mf_set(const struct mf_field *mf,
const union mf_value *value, const union mf_value *mask,
- struct match *match)
+ struct match *match, char **err_str)
{
if (!mask || is_all_ones(mask, mf->n_bytes)) {
-mf_set_value(mf, value, match);
+mf_set_value(mf, value, match, err_str);
return mf->usable_protocols_exact;
} else if (is_all_zeros(mask, mf->n_bytes) && !mf_is_tun_metadata(mf)) {
/* Tunnel metadata matches on the existence of the field itself, so
* it still needs to be encoded even if the value is wildcarded. */
-mf_set_wild(mf, match);
+mf_set_wild(mf, match, err_str);
return OFPUTIL_P_ANY;
}
+if (err_str) {
+*err_str = NULL;
+}
+
switch (mf->id) {
case MFF_RECIRC_ID:
case MFF_CONJ_ID:
@@ -1665,7 +1689,7 @@ mf_set(const struct mf_field *mf,
match_set_tun_tos_masked(match, value->u8, mask->u8);
break;
CASE_MFF_TUN_METADATA:
-tun_metadata_set_match(mf, value, mask, match);
+tun_metadata_set_match(mf, value, mask, match, err_str);
break;
case MFF_METADATA:
@@ -1731,7 +1755,7 @@ mf_set(const struct mf_field *mf,
case MFF_IPV6_LABEL:
if ((mask->be32 & htonl(IPV6_LABEL_MASK)) == htonl(IPV6_LABEL_MASK)) {
-
