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 <[email protected]>
> Signed-off-by: Arun Sharma <[email protected]>
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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
