Message ID | 20140309130005.23270.28970.stgit@Ph0enix |
---|---|
State | Superseded |
Headers | show |
Hi Alvaro, a few coments below about this patch. On 9 March 2014 14:00, Alvaro Neira Ayuso <alvaroneay@gmail.com> wrote: > From: Álvaro Neira Ayuso <alvaroneay@gmail.com> > > Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com> > --- > src/rule.c | 77 +++++++++++++++++++++++++++++++----------------------------- > 1 file changed, 40 insertions(+), 37 deletions(-) > I would suggest to describe this parser change. > diff --git a/src/rule.c b/src/rule.c > index f0cafd3..e7335f8 100644 > --- a/src/rule.c > +++ b/src/rule.c > @@ -540,28 +540,36 @@ int nft_jansson_parse_rule(struct nft_rule *r, json_t *tree, > if (root == NULL) > return -1; > > - if (nft_jansson_parse_family(root, &family, err) != 0) > - goto err; > + if (nft_jansson_node_exist(root, "family")) { > + if (nft_jansson_parse_family(root, &family, err) != 0) > + goto err; > > nft_rule_attr_set_u32(r, NFT_RULE_ATTR_FAMILY, family); > + } > Fix indentation. The nft_rule_attr_set_u32() is now inside the if statement. > - if (nft_jansson_parse_val(root, "handle", NFT_TYPE_U64, &uval64, > - err) < 0) > - goto err; > + if (nft_jansson_node_exist(root, "handle")) { > + if (nft_jansson_parse_val(root, "handle", NFT_TYPE_U64, &uval64, > + err) < 0) Mixed tab/spaces indentation before 'err) < 0)' > @@ -640,39 +648,34 @@ int nft_mxml_rule_parse(mxml_node_t *tree, struct nft_rule *r, > > family = nft_mxml_family_parse(tree, "family", MXML_DESCEND_FIRST, > NFT_XML_MAND, err); > - if (family < 0) > - return -1; > - > - r->family = family; > - r->flags |= (1 << NFT_RULE_ATTR_FAMILY); > + if (family >= 0) { > + r->family = family; > + r->flags |= (1 << NFT_RULE_ATTR_FAMILY); > + } > I would suggest to, while at it, switch to nft_rule_attr_set_xx() functions. IMHO, we will save some LOCs, among other benefits. > - r->table = strdup(table); > - r->flags |= (1 << NFT_RULE_ATTR_TABLE); > + r->table = strdup(table); > + r->flags |= (1 << NFT_RULE_ATTR_TABLE); > + } Just for completeness: remember that for string attributes, the strdup() is done internally, inside nft_rule_attr_set_str(). > if (nft_mxml_num_parse(tree, "handle", MXML_DESCEND_FIRST, BASE_DEC, > - &r->handle, NFT_TYPE_U64, NFT_XML_MAND, err) != 0) > - return -1; > - > - r->flags |= (1 << NFT_RULE_ATTR_HANDLE); > + &r->handle, NFT_TYPE_U64, NFT_XML_MAND, err) >= 0) The line above is 82 characters long. regards!
El 11/03/14 21:16, Arturo Borrero Gonzalez escribió: > Hi Alvaro, > > a few coments below about this patch. > > On 9 March 2014 14:00, Alvaro Neira Ayuso <alvaroneay@gmail.com> wrote: >> From: Álvaro Neira Ayuso <alvaroneay@gmail.com> >> >> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com> >> --- >> src/rule.c | 77 +++++++++++++++++++++++++++++++----------------------------- >> 1 file changed, 40 insertions(+), 37 deletions(-) >> > > I would suggest to describe this parser change. > >> diff --git a/src/rule.c b/src/rule.c >> index f0cafd3..e7335f8 100644 >> --- a/src/rule.c >> +++ b/src/rule.c >> @@ -540,28 +540,36 @@ int nft_jansson_parse_rule(struct nft_rule *r, json_t *tree, >> if (root == NULL) >> return -1; >> >> - if (nft_jansson_parse_family(root, &family, err) != 0) >> - goto err; >> + if (nft_jansson_node_exist(root, "family")) { >> + if (nft_jansson_parse_family(root, &family, err) != 0) >> + goto err; >> >> nft_rule_attr_set_u32(r, NFT_RULE_ATTR_FAMILY, family); >> + } >> > > Fix indentation. The nft_rule_attr_set_u32() is now inside the if statement. > > >> - if (nft_jansson_parse_val(root, "handle", NFT_TYPE_U64, &uval64, >> - err) < 0) >> - goto err; >> + if (nft_jansson_node_exist(root, "handle")) { >> + if (nft_jansson_parse_val(root, "handle", NFT_TYPE_U64, &uval64, >> + err) < 0) > > Mixed tab/spaces indentation before 'err) < 0)' > > >> @@ -640,39 +648,34 @@ int nft_mxml_rule_parse(mxml_node_t *tree, struct nft_rule *r, >> >> family = nft_mxml_family_parse(tree, "family", MXML_DESCEND_FIRST, >> NFT_XML_MAND, err); >> - if (family < 0) >> - return -1; >> - >> - r->family = family; >> - r->flags |= (1 << NFT_RULE_ATTR_FAMILY); >> + if (family >= 0) { >> + r->family = family; >> + r->flags |= (1 << NFT_RULE_ATTR_FAMILY); >> + } >> > > I would suggest to, while at it, switch to nft_rule_attr_set_xx() functions. > IMHO, we will save some LOCs, among other benefits. Yes, i have put this like that for following the same format that you have had. The next time, i'm going to ask before :D. I'm going to change that. > >> - r->table = strdup(table); >> - r->flags |= (1 << NFT_RULE_ATTR_TABLE); >> + r->table = strdup(table); >> + r->flags |= (1 << NFT_RULE_ATTR_TABLE); >> + } > > Just for completeness: > remember that for string attributes, the strdup() is done internally, > inside nft_rule_attr_set_str(). I know that ^^. > >> if (nft_mxml_num_parse(tree, "handle", MXML_DESCEND_FIRST, BASE_DEC, >> - &r->handle, NFT_TYPE_U64, NFT_XML_MAND, err) != 0) >> - return -1; >> - >> - r->flags |= (1 << NFT_RULE_ATTR_HANDLE); >> + &r->handle, NFT_TYPE_U64, NFT_XML_MAND, err) >= 0) > > The line above is 82 characters long. Thanks Arturo for helping me. I'm going to fix all the stuff. Regards Álvaro -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/rule.c b/src/rule.c index f0cafd3..e7335f8 100644 --- a/src/rule.c +++ b/src/rule.c @@ -540,28 +540,36 @@ int nft_jansson_parse_rule(struct nft_rule *r, json_t *tree, if (root == NULL) return -1; - if (nft_jansson_parse_family(root, &family, err) != 0) - goto err; + if (nft_jansson_node_exist(root, "family")) { + if (nft_jansson_parse_family(root, &family, err) != 0) + goto err; nft_rule_attr_set_u32(r, NFT_RULE_ATTR_FAMILY, family); + } - str = nft_jansson_parse_str(root, "table", err); - if (str == NULL) - goto err; + if (nft_jansson_node_exist(root, "table")) { + str = nft_jansson_parse_str(root, "table", err); + if (str == NULL) + goto err; - nft_rule_attr_set_str(r, NFT_RULE_ATTR_TABLE, str); + nft_rule_attr_set_str(r, NFT_RULE_ATTR_TABLE, str); + } - str = nft_jansson_parse_str(root, "chain", err); - if (str == NULL) - goto err; + if (nft_jansson_node_exist(root, "chain")) { + str = nft_jansson_parse_str(root, "chain", err); + if (str == NULL) + goto err; - nft_rule_attr_set_str(r, NFT_RULE_ATTR_CHAIN, str); + nft_rule_attr_set_str(r, NFT_RULE_ATTR_CHAIN, str); + } - if (nft_jansson_parse_val(root, "handle", NFT_TYPE_U64, &uval64, - err) < 0) - goto err; + if (nft_jansson_node_exist(root, "handle")) { + if (nft_jansson_parse_val(root, "handle", NFT_TYPE_U64, &uval64, + err) < 0) + goto err; - nft_rule_attr_set_u64(r, NFT_RULE_ATTR_HANDLE, uval64); + nft_rule_attr_set_u64(r, NFT_RULE_ATTR_HANDLE, uval64); + } if (nft_jansson_node_exist(root, "compat_proto") || nft_jansson_node_exist(root, "compat_flags")) { @@ -640,39 +648,34 @@ int nft_mxml_rule_parse(mxml_node_t *tree, struct nft_rule *r, family = nft_mxml_family_parse(tree, "family", MXML_DESCEND_FIRST, NFT_XML_MAND, err); - if (family < 0) - return -1; - - r->family = family; - r->flags |= (1 << NFT_RULE_ATTR_FAMILY); + if (family >= 0) { + r->family = family; + r->flags |= (1 << NFT_RULE_ATTR_FAMILY); + } table = nft_mxml_str_parse(tree, "table", MXML_DESCEND_FIRST, NFT_XML_MAND, err); - if (table == NULL) - return -1; - - if (r->table) - xfree(r->table); + if (table != NULL) { + if (r->table) + xfree(r->table); - r->table = strdup(table); - r->flags |= (1 << NFT_RULE_ATTR_TABLE); + r->table = strdup(table); + r->flags |= (1 << NFT_RULE_ATTR_TABLE); + } chain = nft_mxml_str_parse(tree, "chain", MXML_DESCEND_FIRST, NFT_XML_MAND, err); - if (chain == NULL) - return -1; - - if (r->chain) - xfree(r->chain); + if (chain != NULL) { + if (r->chain) + xfree(r->chain); - r->chain = strdup(chain); - r->flags |= (1 << NFT_RULE_ATTR_CHAIN); + r->chain = strdup(chain); + r->flags |= (1 << NFT_RULE_ATTR_CHAIN); + } if (nft_mxml_num_parse(tree, "handle", MXML_DESCEND_FIRST, BASE_DEC, - &r->handle, NFT_TYPE_U64, NFT_XML_MAND, err) != 0) - return -1; - - r->flags |= (1 << NFT_RULE_ATTR_HANDLE); + &r->handle, NFT_TYPE_U64, NFT_XML_MAND, err) >= 0) + r->flags |= (1 << NFT_RULE_ATTR_HANDLE); if (nft_mxml_num_parse(tree, "compat_proto", MXML_DESCEND_FIRST, BASE_DEC, &r->compat.proto, NFT_TYPE_U32,