On Wed, Apr 26, 2023 at 8:57 PM Simon Horman <[email protected]> wrote:
>
> On Mon, Apr 24, 2023 at 02:01:03PM +0800, 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]>
>
> Hi Wan Junjie,
>
> thanks for your patch.
>
Hi Simon,
Thank you for your review.
> > ---
> > include/openvswitch/vconn.h | 2 +-
> > lib/vconn.c | 23 ++++++++++++++---------
> > utilities/ovs-ofctl.c | 19 +++++++++++++------
> > 3 files changed, 28 insertions(+), 16 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..a5f6177ec 100644
> > --- a/lib/vconn.c
> > +++ b/lib/vconn.c
> > @@ -926,23 +926,28 @@ 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) {
> > struct ofpbuf *request;
> > + int error = 0;
> >
> > LIST_FOR_EACH_POP (request, list_node, requests) {
> > - int error;
> > + struct ofpbuf *reply;
> > +
> > + error = vconn_transact_noreply(vconn, request, &reply);
> >
>
> nit: no blank line here please
>
OK
> > - error = vconn_transact_noreply(vconn, request, replyp);
> > - if (error || *replyp) {
> > - ofpbuf_list_delete(requests);
> > + if (error && !reply) {
> > return error;
> > }
> > + if (reply) {
> > + ovs_list_push_back(replies, &reply->list_node);
> > + }
> > }
> >
> > - *replyp = NULL;
> > + if (!ovs_list_is_empty(replies)) {
> > + return error;
>
> If there has been a reply then we get here.
> And return the most recent value of error which,
> by my reading, may not relate to any of the replies.
> Is this intentional?
>
As I can see in vconn_transact_noreply, there should be two return patterns.
One is an error and no reply, as error means a vconn is broken. In this case we
should stop further requests as they will fail anyway.
The other is no error and a valid reply, though the reply means a
protocol error.
The original code take both patterns as errors and stop further
requests. This change
will take the second pattern as a normal response.
So if it reaches the code of return error here, usually it means error = 0.
> > + }
> > return 0;
> > }
> >
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > index 24d0941cf..a84551672 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) {
> > +
>
> nit: no blank line here please
>
OK
> > + 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);
> > + tables_to_show(vconn_get_name(vconn)), verbosity + 2);
>
> nit: the change to the line immediately above seems to be whitepace only.
> I don't think it belongs in this patch.
>
code here move exit to a later place. Newline here is not intensional.
> > + ofpbuf_delete(reply);
> > }
> > - ofpbuf_delete(reply);
> > + exit(1);
> > }
> >
> > /* Frees the error messages as they are printed. */
>
> ...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
