[ovs-dev] ovsdb-idl: Properly handle conditional monitor update error

When generating conditional monitoring update request, current code
failed to update idl's 'request-id'.  This bug causes the reply
message of the update request, regardless an ACK or a NACK, be
logged as an unexpected message at the debug level and ignored by
the core idl logic.

In addition, the idl should not generate another conditional
monitoring update request when there is an outstanding request.
So that the requests and their reply are properly serialized.

Signed-off-by: Andy Zhou <azhou@ovn.org>
---
 lib/ovsdb-idl.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

I have posted an updated version of this patch at:

https://patchwork.ozlabs.org/patch/707398/

On Fri, Dec 16, 2016 at 4:55 PM, Andy Zhou <azhou@ovn.org> wrote:

> When generating conditional monitoring update request, current code
> failed to update idl's 'request-id'.  This bug causes the reply
> message of the update request, regardless an ACK or a NACK, be
> logged as an unexpected message at the debug level and ignored by
> the core idl logic.
>
> In addition, the idl should not generate another conditional
> monitoring update request when there is an outstanding request.
> So that the requests and their reply are properly serialized.
>
> Signed-off-by: Andy Zhou <azhou@ovn.org>
> ---
>  lib/ovsdb-idl.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index 8ce228d..fa659a8 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -454,8 +454,11 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
>                  idl->schema = NULL;
>                  break;
>
> -            case IDL_S_MONITORING:
>              case IDL_S_MONITORING_COND:
> +                /* Conditional monitor clauses were updated. */
> +                break;
> +
> +            case IDL_S_MONITORING:
>              case IDL_S_NO_SCHEMA:
>              default:
>                  OVS_NOT_REACHED();
> @@ -494,14 +497,22 @@ ovsdb_idl_run(struct ovsdb_idl *idl)
>                  idl->state = IDL_S_MONITOR_REQUESTED;
>              }
>          } else if (msg->type == JSONRPC_ERROR
> +                   && idl->state == IDL_S_MONITORING_COND
> +                   && idl->request_id
> +                   && json_equal(idl->request_id, msg->id)) {
> +            json_destroy(idl->request_id);
> +            idl->request_id = NULL;
> +            VLOG_ERR("%s: conditional monitor update failed",
> +                     jsonrpc_session_get_name(idl->session));
> +        } else if (msg->type == JSONRPC_ERROR
>                     && idl->state == IDL_S_SCHEMA_REQUESTED
>                     && idl->request_id
>                     && json_equal(idl->request_id, msg->id)) {
> -                json_destroy(idl->request_id);
> -                idl->request_id = NULL;
> -                VLOG_ERR("%s: requested schema not found",
> -                         jsonrpc_session_get_name(idl->session));
> -                idl->state = IDL_S_NO_SCHEMA;
> +            json_destroy(idl->request_id);
> +            idl->request_id = NULL;
> +            VLOG_ERR("%s: requested schema not found",
> +                     jsonrpc_session_get_name(idl->session));
> +            idl->state = IDL_S_NO_SCHEMA;
>          } else if ((msg->type == JSONRPC_ERROR
>                      || msg->type == JSONRPC_REPLY)
>                     && ovsdb_idl_txn_process_reply(idl, msg)) {
> @@ -1021,8 +1032,11 @@ ovsdb_idl_send_cond_change(struct ovsdb_idl *idl)
>      struct json *params, *json_uuid;
>      struct jsonrpc_msg *request;
>
> +    /* When 'idl-request_id' is not NULL, there is an outstanding
> +     * conditional monitoring update request that we have not heard
> +     * from the server yet. Don't generate another request in this case.
> */
>      if (!idl->cond_changed || !jsonrpc_session_is_connected(idl->session)
> ||
> -        idl->state != IDL_S_MONITORING_COND) {
> +        idl->state != IDL_S_MONITORING_COND || idl->request_id) {
>          return;
>      }
>
> @@ -1058,7 +1072,8 @@ ovsdb_idl_send_cond_change(struct ovsdb_idl *idl)
>          params = json_array_create_3(json_uuid, json_string_create(uuid),
>                                       monitor_cond_change_requests);
>
> -        request = jsonrpc_create_request("monitor_cond_change", params,
> NULL);
> +        request = jsonrpc_create_request("monitor_cond_change", params,
> +                                         &idl->request_id);
>          jsonrpc_session_send(idl->session, request);
>      }
>      idl->cond_changed = false;
> --
> 2.7.4
>
>