On Tue, Nov 4, 2014 at 12:48 PM, Ben Pfaff <[email protected]> wrote:
> On Thu, Oct 23, 2014 at 03:52:33PM -0700, Andy Zhou wrote:
>> OVS userspace are backward compatible with older Linux kernel modules.
>> However, not having the most up-to-date datapath kernel modules can
>> some times lead to user confusion. Storing the datapath version in
>> OVSDB allows management software to check and optionally provide
>> notifications to users.
>>
>> Signed-off-by: Andy Zhou <[email protected]>
>>
>> ----
>> v1->v2: Get version string from dpif_class.
>> V2->V3: Fix missspelling of 'datapath' in comments
>> V3->v4: Fix vswitch.xml description to referece Open vSwitch instead
>> of OpenFlow
>> V4->V5: Add handlinf for possible empty kernel module version file.
>> Update OVSDB with empty string when datapath version cannot
>> be determined.
>
> Thanks for working on this.
>
> I've been thinking about how dpif-netdev.c being built-in forces some
> nasty contortions. It's really not very nice how ovs-vswitchd has to
> read its own version number out of the database to put into the
> dpif-netdev version number (because the startup script is what puts it
> there). There might be better ways to handle that (maybe we could
> move it out of the startup script), but I think that in the end it's
> better to just use a "version" string that indicates that that the
> datapath is built in. That answers the real question that the
> management software has (whether ovs-vswitchd and the datapath are the
> same version) and it simplifies the code. It also side-steps
> questions of what happens if the version number in the database
> changes at runtime (I think that currently it's cached and so the
> datapath version won't get updated.) So: I think it would be better
> to return a string that says the datapath is built-in. I suggest
> "<built-in>". (This should be documented.)
Good idea. This also makes the patch simpler.
>
> While we're at it, I think that it would be better to have a string
> that represents an unknown version, rather than just the empty string,
> because the latter makes it look like the database column simply
> hasn't been updated yet. I suggest "<unknown>".
>
> br_refresh_datapath_info() looks more complicated than I think it
> really is. How about this:
>
> static void
> br_refresh_datapath_info(struct bridge *br)
> {
> const char *version;
>
> version = (br->ofproto && br->ofproto->ofproto_class->get_datapath_version
> ? br->ofproto->ofproto_class->get_datapath_version(br->ofproto)
> : NULL);
>
> ovsrec_bridge_set_datapath_version(br->cfg,
> version ? version : "<unknown>");
> }
O.K. I will use this version instead. Will send out v6 soon.
>
> Thanks,
>
> Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
