On 5/6/23 05:20, Wan Junjie via dev wrote:
> ovs-save will do replace-flows for ovs restart. For some reason, ovs
> could have an invalid action, like invalid meter. Then adding flows
> will be stopped at the position of the first invalid flow. This could
> happen when ovs restart and vm related resource allocatting or destructing
> at the same time.
>
> This patch will skip the invalid flows and continue the process so
> all valid flows will be added during the ovs restart. An destructed vm
> will not affect those normal vm traffic.
>
> Signed-off-by: Wan Junjie <[email protected]>
>
> ---
> v2:
> handle vconn_transact_noreply's return error and reply seperately
> code style
> ---
> include/openvswitch/vconn.h | 2 +-
> lib/vconn.c | 20 ++++++++++----------
> utilities/ovs-ofctl.c | 15 +++++++++++----
> 3 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/include/openvswitch/vconn.h b/include/openvswitch/vconn.h
> index 5f69c732b..781c59075 100644
> --- a/include/openvswitch/vconn.h
> +++ b/include/openvswitch/vconn.h
> @@ -57,7 +57,7 @@ int vconn_recv_xid(struct vconn *, ovs_be32 xid, struct
> ofpbuf **);
> int vconn_transact(struct vconn *, struct ofpbuf *, struct ofpbuf **);
> int vconn_transact_noreply(struct vconn *, struct ofpbuf *, struct ofpbuf
> **);
> int vconn_transact_multiple_noreply(struct vconn *, struct ovs_list
> *requests,
> - struct ofpbuf **replyp);
> + struct ovs_list *replies);
> int vconn_transact_multipart(struct vconn *, struct ovs_list *request,
> struct ovs_list *replies);
>
> diff --git a/lib/vconn.c b/lib/vconn.c
> index b55676227..bafd2657b 100644
> --- a/lib/vconn.c
> +++ b/lib/vconn.c
> @@ -926,23 +926,23 @@ vconn_transact_noreply(struct vconn *vconn, struct
> ofpbuf *request,
> /* vconn_transact_noreply() for a list of "struct ofpbuf"s, sent one by one.
> * All of the requests on 'requests' are always destroyed, regardless of the
> * return value. */
> -int
> -vconn_transact_multiple_noreply(struct vconn *vconn, struct ovs_list
> *requests,
> - struct ofpbuf **replyp)
> -{
> +int vconn_transact_multiple_noreply(struct vconn *vconn,
> + struct ovs_list *requests,
> + struct ovs_list *replies) {
Please, don't change the style. Function name in the implementation should
be on a separate line and the '{' should also be on a separate line.
> struct ofpbuf *request;
> + int error = 0;
>
> LIST_FOR_EACH_POP (request, list_node, requests) {
> - int error;
> -
> - error = vconn_transact_noreply(vconn, request, replyp);
> - if (error || *replyp) {
> - ofpbuf_list_delete(requests);
> + struct ofpbuf *reply;
An empty line is needed here. (Simon's comment was about a different one.)
> + error = vconn_transact_noreply(vconn, request, &reply);
> + if (error) {
> return error;
> }
> + if (reply) {
> + ovs_list_push_back(replies, &reply->list_node);
So, the reply is added only if there is one. This way we're loosing
the order and there is no way for the caller to understand which
request this reply is for. Even if OVS itself doesn't use this,
it's a public API, so we should provide some consistency, e.g. insert
empty replies for places where there was no reply.
OTOH, we may not need to touch this function at all. See below.
> + }
> }
>
> - *replyp = NULL;
> return 0;
> }
>
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 24d0941cf..83374e6c4 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -715,18 +715,25 @@ dump_trivial_transaction(const char *vconn_name, enum
> ofpraw ofpraw)
> static void
> transact_multiple_noreply(struct vconn *vconn, struct ovs_list *requests)
> {
> + struct ovs_list replies;
> struct ofpbuf *reply;
>
> - run(vconn_transact_multiple_noreply(vconn, requests, &reply),
> + ovs_list_init(&replies);
> +
> + run(vconn_transact_multiple_noreply(vconn, requests, &replies),
> "talking to %s", vconn_get_name(vconn));
> - if (reply) {
> + if (ovs_list_is_empty(&replies)) {
> + return;
> + }
> +
> + LIST_FOR_EACH_POP (reply, list_node, &replies) {
> ofp_print(stderr, reply->data, reply->size,
> ports_to_show(vconn_get_name(vconn)),
> tables_to_show(vconn_get_name(vconn)),
> verbosity + 2);
> - exit(1);
> + ofpbuf_delete(reply);
> }
AFAICT, the main point of calling vconn_transact_multiple_noreply here
was to avoid a loop. But now we have a loop anyway. Can we just call
vconn_transact_noreply() in a loop then and handle errors as we go?
This way we'll not need to touch vconn_transact_multiple_noreply() code.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
