[ovs-dev] [PATCH] ovs-vsctl: Print error for invalid controller and manager targets.

Justin Pettit jpettit at nicira.com
Wed Oct 12 16:02:31 UTC 2011


On Oct 12, 2011, at 6:53 AM, Ben Pfaff wrote:

> If we add a new way to connect to a controller, then we have to add it
> here too or no one can (easily) use that new way.  The list of
> possibilities is not complete, since you can also use pssl, ptcp, and
> punix.  You can fix that by using vconn_verify_name() and
> pvconn_verify_name().

Whoops.  I remember that you can specify listeners, but I got focused on the list presented in the man page.  I'll update the man page and use those functions.

> I'd like ovs-vsctl to be able to control somewhat different versions of
> OVS (certainly this goal is not fully achieved), so that one could add,
> for example, an SCTP controller to a (hypothetical) newer version of OVS
> that supports it with an older version of ovs-vsctl.  So I'd also reduce
> the fatal error to a warning.
> 
> I bet the next question is going to be "how do I get the list of valid
> possibilities for the log message" so I'll note that I don't think we
> have to list all of them but just say that the syntax is incorrect and
> suggest a few likely possibilities.

Sounds good.

>> @@ -2168,6 +2174,15 @@ insert_managers(struct vsctl_context *ctx, char *targets[], size_t n)
>>     /* Insert each manager in a new row in Manager table. */
>>     managers = xmalloc(n * sizeof *managers);
>>     for (i = 0; i < n; i++) {
>> +        if (strncmp(targets[i], "ssl:", 4)
>> +                && strncmp(targets[i], "tcp:", 4)
>> +                && strncmp(targets[i], "unix:", 5)
>> +                && strncmp(targets[i], "pssl:", 5)
>> +                && strncmp(targets[i], "ptcp:", 5)
>> +                && strncmp(targets[i], "punix:", 6)) {
>> +            vsctl_fatal("target must begin with \"[p]ssl:\", \"[p]tcp:\", "
>> +                        "or \"[p]unix:\"");
>> +        }
> 
> Similar comments here, except that stream_verify_name() and
> pstream_verify_name() are the right functions to call.

Thanks for the review.  I'll send out a revised version later.

--Justin





More information about the dev mailing list