Ben Pfaff <[email protected]> wrote on 01/06/2016 09:36:37 PM:
> On Tue, May 17, 2016 at 05:27:00PM +0300, Liran Schour wrote:
> > Change ovsdb_condition to be a 3-element json array or a boolean
value.
> > Conditions utilities will be used later for conditional monitoring.
> >
> > Signed-off-by: Liran Schour <[email protected]>
>
> I believe that this actually extends the OVSDB protocol. When we make
> extensions, we document them in ovsdb/ovsdb-server.1.in, in the section
> titled SPECIFICATIONS, so please add a description there.
>
Will move the specification of condition to this patch.
> The code in ovsdb_clause_from_json() for converting JSON_TRUE into
> OVSDB_F_TRUE and JSON_FALSE into OVSDB_F_FALSE seems awfully indirect.
> Why not just use the enums directly?
> + if (json->type == JSON_TRUE || json->type == JSON_FALSE) {
> + function_name = (json->type == JSON_TRUE) ? "true" : "false";
> + error = ovsdb_function_from_string(function_name,
&clause->function);
>
Right. Will fix that.
> Please use capital letters and punctuation in comments:
> + /* column and arg fields are not being used with boolean
function
> + * use dummy values */
>
Sure.
> It should not be possible to hit these cases in
> ovsdb_clause_from_json(), so please use OVS_NOT_REACHED():
> + case OVSDB_F_TRUE:
> + case OVSDB_F_FALSE:
>
Will fix that.
> There are a couple of instances of
> <boolean-expression> ? true : false
> in this patch. Please just drop the ?: part, it's a no-op, e.g.
> + return json_boolean_create(clause->function == OVSDB_F_TRUE ?
> + true : false);
> becomes
> + return json_boolean_create(clause->function == OVSDB_F_TRUE);
>
Will change that.
Thanks for the review,
- Liran
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
