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

[Andy Zhou]

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 at 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 at 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
>
>