[ovs-dev] [PATCH 09/10] dpif: Don't synchronize flow_dump_next() status.

[Ben Pfaff]

On Tue, Jan 14, 2014 at 10:41:30AM -0800, Joe Stringer wrote:
> On 13 January 2014 15:25, Ben Pfaff <blp at nicira.com> wrote:
> > On Fri, Jan 10, 2014 at 11:43:14AM -0800, Joe Stringer wrote:
> > > In an earlier incarnation of dpif, the interface specified that if
> > > flow_dump_next() returned an error code, then the function must not be
> > > called again in the same dump operation. As such, the dpif itself needed
> > > to cache the error status to ensure that it did not make such calls.
> > > Recent changes have relaxed this restriction, making it the
> > > responsibility of the dpif implementation to track flow dump error
> > > status.
> > >
> > > This patch ensures that dpif_flow_dump_next() does not attempt to finish
> > > the flow dump, as it will be the responsibility of just one thread to do
> > > this. Furthermore, we no longer update 'dump->status' in this function,
> > > so that even if one thread finishes processing flows in its buffer (and
> > > handles the final flow), it will not prevent other callers from
> > > processing the remaining flows in their buffers.
> > >
> > > After this patch, the error code that dpif_flow_dump_next() returns is
> > > only significant for the current state and buffer. As before, the status
> > > of the entire flow dump operation can be obtained by calling
> > > dpif_flow_dump_done().
> > >
> > > Signed-off-by: Joe Stringer <joestringer at nicira.com>
> >
> > Doesn't think cause errors generated by flow_dump_next implementations
> > to be lost?  e.g. dpif_linux_flow_dump_next() can return errors
> > produced by dpif_linux_flow_from_ofpbuf().
> >
> 
> Yes, I missed this. My intention was to push error status tracking down to
> the dpif implementation. In this case, dpif_flow_dump_next() only
> determines whether to continue at the moment, while dpif_flow_dump_done()
> provides the error status for the entire operation by calling the
> flow_dump_done() implementation. This means that dpif-linux should be
> storing this error condition to return at the end of the entire flow_dump
> (and potentially logging the error when it occurs). Do you have a
> preference on this?

I think your idea to push it down to the implementation is fine.

> By the way, somewhere along the line we ended up with a really silly
> > loop in dpif_linux_flow_from_ofpbuf() (the "while" condition can never
> > be true):
> >
> >     do {
> >         if (!nl_dump_next(&state->dump, &buf, buffer)) {
> >             return EOF;
> >         }
> >
> >         error = dpif_linux_flow_from_ofpbuf(&flow, &buf);
> >         if (error) {
> >             return error;
> >         }
> >     } while (error);
> >
> 
> Right. The loop was there to handle failures when fetching actions, but
> I've removed the actions. I'll make sure to tidy this up when I go back
> over the actions patch.

Thanks.