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.manc...@rackspace.com> > Signed-off-by: Arun Sharma <arun.sha...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev