Thanks for your review.
On 2/7/11 1:22 PM, Ben Pfaff wrote:
> In the manpage, lib/vconn-active.man lists ways to connect to OpenFlow
> switches or controllers. Instead, we should list ways to connect to
> OSVDB clients, which are listed in ovsdb/remote-passive.man and
> ovsdb/remote-active.man.
Oops, good catch. Thanks, I've fixed it with:
@@ -376,7 +380,8 @@ Sets the configured manager target or targets. Each
\fItarget\fR may
use any of the following forms:
.
.RS
-.so lib/vconn-active.man
+.so ovsdb/remote-active.man
+.so ovsdb/remote-passive.man
.RE
.
.SS "SSL Configuration"
> I'd be a little more explicit about what the managers commands do.
> Maybe something like this:
>
> These commands manipulate the \fBmanagers\fR and
> \fBmanager_options\fR columns in the \fBOpen_vSwitch\fR table and
> rows in the \fBManagers\fR table. When \fBovsdb\-server\fR is
> configured to use those rows and columns for OVSDB connections, as
> described in \fBINSTALL.Linux\fR and in the startup scripts provided
> with Open vSwitch, this allows the administrator to use
> \fBovs\-vsctl\fR to configure database connections.
Looks great, so I've used it verbatim.
> From pre_manager(), I would not call pre_get_info(). It should not be
> necessary, because none of the manager operations call get_info().
Thanks, missed this one.
> In cmd_get_manager() I would use svec_sort_unique(), instead of
> svec_sort(), because it is valid for a manager to appear in both places
> and in such a case we only want to print it once.
Another good catch. Done.
> In delete_managers(), I would only delete the rows in the Managers table
> that the manager_options column referred to, instead of all the rows.
Hmm, ok. I don't see anything useful about keeping dangling Manager rows
around, but I can understand the principle. Changed:
@@ -1924,19 +1922,18 @@ static void
delete_managers(const struct vsctl_context *ctx)
{
const struct ovsrec_open_vswitch *ovs = ctx->ovs;
- const struct ovsdb_idl *idl = ctx->idl;
- const struct ovsrec_manager *row, *next;
+ size_t i;
/* Delete manager targets in deprecated 'managers' column. */
ovsrec_open_vswitch_set_managers(ovs, NULL, 0);
+ /* Delete Manager rows pointed to by 'manager_options' column. */
+ for (i = 0; i < ovs->n_manager_options; i++) {
+ ovsrec_manager_delete(ovs->manager_options[i]);
+ }
+
/* Delete 'Manager' row refs in 'manager_options' column. */
ovsrec_open_vswitch_set_manager_options(ovs, NULL, 0);
-
- /* Delete all rows in 'Manager' table. */
- OVSREC_MANAGER_FOR_EACH_SAFE (row, next, idl) {
- ovsrec_manager_delete(row);
- }
}
static void
> In insert_managers(), I would also put each manager in the 'managers'
> column of Open_vSwitch, for compatibility with any software that expects
> the list of managers to appear there.
Sensible. Done:
@@ -1954,12 +1951,17 @@ insert_managers(struct vsctl_context *ctx, char
*targets[], size_t n)
struct ovsrec_manager **managers;
size_t i;
+ /* Store in deprecated 'manager' column. */
+ ovsrec_open_vswitch_set_managers(ctx->ovs, targets, n);
+
+ /* Insert each manager in a new row in Manager table. */
managers = xmalloc(n * sizeof *managers);
for (i = 0; i < n; i++) {
managers[i] = ovsrec_manager_insert(ctx->txn);
ovsrec_manager_set_target(managers[i], targets[i]);
}
+ /* Store uuids of new Manager rows in 'manager_options' column. */
ovsrec_open_vswitch_set_manager_options(ctx->ovs, managers, n);
free(managers);
}
> I suggest adding a test to tests/ovs-vsctl.at. You can follow the form
> of the "controllers" test that is already there.
Ok, how does this look?
diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index a05e805..bb49450 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -504,6 +504,32 @@ OVS_VSCTL_CLEANUP
AT_CLEANUP
dnl ----------------------------------------------------------------------
+AT_BANNER([ovs-vsctl unit tests -- manager commands])
+
+AT_SETUP([managers])
+AT_KEYWORDS([manager ovs-vsctl])
+OVS_VSCTL_SETUP
+AT_CHECK([RUN_OVS_VSCTL_TOGETHER(
+ [del-manager],
+ [get-manager],
+ [set-manager tcp:4.5.6.7],
+ [get-manager],
+ [set-manager tcp:8.9.10.11 tcp:5.4.3.2],
+ [get-manager],
+ [del-manager],
+ [get-manager])], [0], [
+
+
+tcp:4.5.6.7
+
+tcp:5.4.3.2\ntcp:8.9.10.11
+
+
+], [], [OVS_VSCTL_CLEANUP])
+OVS_VSCTL_CLEANUP
+AT_CLEANUP
+
+dnl ----------------------------------------------------------------------
AT_BANNER([ovs-vsctl unit tests -- database commands])
AT_SETUP([database commands -- positive checks])
> Thanks,
>
> Ben.
Thank you!
-Andrew
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
