[ovs-dev] [PATCH] ovs-vsctl: Add "--all" option for "destroy" command in ovs-vsctl.

[Ben Pfaff]

On Tue, May 15, 2012 at 01:51:49AM -0700, Arun Sharma wrote:
> Adds the ability to delete all records from table. This will help
> users to destroy all records from Qos or Queue table using single
> command rather then current method.
> 
> Feature #11306
> Suggested-by: Kevin Mancuso <kevin.mancuso at rackspace.com>
> Signed-off-by: Arun Sharma <arun.sharma at calsoftinc.com>

Thanks Arun.  I have some comments.

The test is a good start, but so far it only verifies that the commands
execute without error.  To also verify that the commands have the
intended effect, I would expect there to be additional ovs-vsctl
commands to list the contents of the tables after each modification.

I think that the documentation for the destroy command would be clearer
if we documented it as two separate commands: one without --all that
accepts records as arguments, and one with --all that does not accept
records as arguments.  The first command would have the existing
summary:

.IP "\fR[\fB\-\-if\-exists\fR] \fBdestroy \fItable record\fR..."

The second command would have the following summary:

.IP "\fB\-\-all destroy \fItable\fR"

In cmd_destroy(), I would report an error and abort, with vsctl_fatal(),
if --all was used but some record ids were specified anyway, or if both
--all and --if-exists were specified.

This loop is unsafe because ovsdb_idl_txn_delete(row) can destroy 'row',
so that passing it to ovsdb_idl_next_row() is a use-after-free error:

        for (row = ovsdb_idl_first_row(ctx->idl, table->class);
             row; row = ovsdb_idl_next_row(row)) {
             ovsdb_idl_txn_delete(row);
        }

I suggest finding the next row before deleting the current one.

Thanks,

Ben.