| Message ID | 20200218203911.30502-1-jwcart2@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libsepol: Use ebitmap_length() to check for an empty ebitmap | expand |
On Tue, Feb 18, 2020 at 9:45 PM James Carter <jwcart2@tycho.nsa.gov> wrote: > When checking whether or not an ebitmap has any bits set, use > ebitmap_length() instead of ebitmap_cardinality(). > > There is no need to find out how many bits are set, if all that is > needed is to determine if any bits are set at all. > > Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> > --- > libsepol/src/kernel_to_cil.c | 10 +++++----- > libsepol/src/kernel_to_conf.c | 8 ++++---- > libsepol/src/module_to_cil.c | 16 ++++++++-------- > 3 files changed, 17 insertions(+), 17 deletions(-) Thanks, this looks good! Although I'm thinking if we shouldn't add a specific function for this, e.g.: static inline bool ebitmap_is_empty(ebitmap_t *e) { return ebitmap_length(e) == 0; } ...because ebitmap_length() is kind of an implementation detail and it is easy to confuse ebitmap_length() and ebitmap_cardinality(). Note there are already some existing callers of ebitmap_length() that would also need converting to ebitmap_is_empty() in that case. <diff snipped>
On 2/18/20 3:39 PM, James Carter wrote: > When checking whether or not an ebitmap has any bits set, use > ebitmap_length() instead of ebitmap_cardinality(). > > There is no need to find out how many bits are set, if all that is > needed is to determine if any bits are set at all. > > Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> > --- > diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c > index e20c3d44..b1cbef08 100644 > --- a/libsepol/src/module_to_cil.c > +++ b/libsepol/src/module_to_cil.c > @@ -2149,7 +2149,7 @@ static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN > } > } > > - if (ebitmap_cardinality(&role->dominates) > 1) { > + if (ebitmap_length(&role->dominates) > 1) { > log_err("Warning: role 'dominance' statement unsupported in CIL. Dropping from output."); > } Noticed that this test differs from the rest, checking > 1 rather than just comparing with 0. Not sure if it matters but ebitmap_length() will be > 1 if role->dominates is non-empty even if it only has one bit set. So maybe this one is supposed to really be ebitmap_cardinality()?
On 2/19/20 3:25 AM, Ondrej Mosnacek wrote: > On Tue, Feb 18, 2020 at 9:45 PM James Carter <jwcart2@tycho.nsa.gov> wrote: >> When checking whether or not an ebitmap has any bits set, use >> ebitmap_length() instead of ebitmap_cardinality(). >> >> There is no need to find out how many bits are set, if all that is >> needed is to determine if any bits are set at all. >> >> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> >> --- >> libsepol/src/kernel_to_cil.c | 10 +++++----- >> libsepol/src/kernel_to_conf.c | 8 ++++---- >> libsepol/src/module_to_cil.c | 16 ++++++++-------- >> 3 files changed, 17 insertions(+), 17 deletions(-) > > Thanks, this looks good! Although I'm thinking if we shouldn't add a > specific function for this, e.g.: > > static inline bool ebitmap_is_empty(ebitmap_t *e) > { > return ebitmap_length(e) == 0; > } > > ...because ebitmap_length() is kind of an implementation detail and it > is easy to confuse ebitmap_length() and ebitmap_cardinality(). Note > there are already some existing callers of ebitmap_length() that would > also need converting to ebitmap_is_empty() in that case. > > <diff snipped> > I think ebitmap_is_empty() is a good idea, but I think a macro will work fine. #define ebitmap_is_empty(e) (((e)->highbit) == 0) Jim
On 2/19/20 8:30 AM, Stephen Smalley wrote: > On 2/18/20 3:39 PM, James Carter wrote: >> When checking whether or not an ebitmap has any bits set, use >> ebitmap_length() instead of ebitmap_cardinality(). >> >> There is no need to find out how many bits are set, if all that is >> needed is to determine if any bits are set at all. >> >> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> >> --- > >> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c >> index e20c3d44..b1cbef08 100644 >> --- a/libsepol/src/module_to_cil.c >> +++ b/libsepol/src/module_to_cil.c >> @@ -2149,7 +2149,7 @@ static int role_to_cil(int indent, struct policydb *pdb, >> struct avrule_block *UN >> } >> } >> - if (ebitmap_cardinality(&role->dominates) > 1) { >> + if (ebitmap_length(&role->dominates) > 1) { >> log_err("Warning: role 'dominance' statement unsupported in CIL. >> Dropping from output."); >> } > > Noticed that this test differs from the rest, checking > 1 rather than just > comparing with 0. Not sure if it matters but ebitmap_length() will be > 1 if > role->dominates is non-empty even if it only has one bit set. So maybe this one > is supposed to really be ebitmap_cardinality()? > You are right. I misread this one. Jim
On Wed, Feb 19, 2020 at 3:54 PM jwcart2 <jwcart2@tycho.nsa.gov> wrote: > On 2/19/20 3:25 AM, Ondrej Mosnacek wrote: > > On Tue, Feb 18, 2020 at 9:45 PM James Carter <jwcart2@tycho.nsa.gov> wrote: > >> When checking whether or not an ebitmap has any bits set, use > >> ebitmap_length() instead of ebitmap_cardinality(). > >> > >> There is no need to find out how many bits are set, if all that is > >> needed is to determine if any bits are set at all. > >> > >> Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> > >> --- > >> libsepol/src/kernel_to_cil.c | 10 +++++----- > >> libsepol/src/kernel_to_conf.c | 8 ++++---- > >> libsepol/src/module_to_cil.c | 16 ++++++++-------- > >> 3 files changed, 17 insertions(+), 17 deletions(-) > > > > Thanks, this looks good! Although I'm thinking if we shouldn't add a > > specific function for this, e.g.: > > > > static inline bool ebitmap_is_empty(ebitmap_t *e) > > { > > return ebitmap_length(e) == 0; > > } > > > > ...because ebitmap_length() is kind of an implementation detail and it > > is easy to confuse ebitmap_length() and ebitmap_cardinality(). Note > > there are already some existing callers of ebitmap_length() that would > > also need converting to ebitmap_is_empty() in that case. > > > > <diff snipped> > > > > I think ebitmap_is_empty() is a good idea, but I think a macro will work fine. > > #define ebitmap_is_empty(e) (((e)->highbit) == 0) I personally try to use inline functions rather than macros whenever possible - they are type-checked and consistent with the rest of the C code. Using macro where a function would suffice is kind of a hack, IMO. But in this very simple case it doesn't make much practical difference, so I'll leave it to your decision (unless other maintainers also object).
diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c index ca2e4a9b..28577faf 100644 --- a/libsepol/src/kernel_to_cil.c +++ b/libsepol/src/kernel_to_cil.c @@ -1101,7 +1101,7 @@ static int write_sensitivitycategory_rules_to_cil(FILE *out, struct policydb *pd } if (level->isalias) continue; - if (ebitmap_cardinality(&level->level->cat) > 0) { + if (ebitmap_length(&level->level->cat) > 0) { cats = cats_ebitmap_to_str(&level->level->cat, pdb->p_cat_val_to_name); sepol_printf(out, "(sensitivitycategory %s %s)\n", name, cats); free(cats); @@ -1502,7 +1502,7 @@ static int write_type_attribute_sets_to_cil(FILE *out, struct policydb *pdb) if (attr->flavor != TYPE_ATTRIB) continue; name = pdb->p_type_val_to_name[i]; typemap = &pdb->attr_type_map[i]; - if (ebitmap_cardinality(typemap) == 0) continue; + if (ebitmap_length(typemap) == 0) continue; types = ebitmap_to_str(typemap, pdb->p_type_val_to_name, 1); if (!types) { rc = -1; @@ -1879,7 +1879,7 @@ static char *level_to_str(struct policydb *pdb, struct mls_level *level) char *sens_str = pdb->p_sens_val_to_name[level->sens - 1]; char *cats_str; - if (ebitmap_cardinality(cats) > 0) { + if (ebitmap_length(cats) > 0) { cats_str = cats_ebitmap_to_str(cats, pdb->p_cat_val_to_name); level_str = create_str("(%s %s)", 2, sens_str, cats_str); free(cats_str); @@ -2188,7 +2188,7 @@ static int write_role_decl_rules_to_cil(FILE *out, struct policydb *pdb) goto exit; } types = &role->types.types; - if (types && (ebitmap_cardinality(types) > 0)) { + if (types && (ebitmap_length(types) > 0)) { rc = strs_init(&type_strs, pdb->p_types.nprim); if (rc != 0) { goto exit; @@ -2373,7 +2373,7 @@ static int write_user_decl_rules_to_cil(FILE *out, struct policydb *pdb) } roles = &user->roles.roles; - if (roles && (ebitmap_cardinality(roles) > 0)) { + if (roles && (ebitmap_length(roles) > 0)) { rc = strs_init(&role_strs, pdb->p_roles.nprim); if (rc != 0) { goto exit; diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c index b4966162..cf42f94c 100644 --- a/libsepol/src/kernel_to_conf.c +++ b/libsepol/src/kernel_to_conf.c @@ -1090,7 +1090,7 @@ static int write_level_rules_to_conf(FILE *out, struct policydb *pdb) } if (level->isalias) continue; - if (ebitmap_cardinality(&level->level->cat) > 0) { + if (ebitmap_length(&level->level->cat) > 0) { cats = cats_ebitmap_to_str(&level->level->cat, pdb->p_cat_val_to_name); sepol_printf(out, "level %s:%s;\n", name, cats); free(cats); @@ -1859,7 +1859,7 @@ static char *level_to_str(struct policydb *pdb, struct mls_level *level) char *sens_str = pdb->p_sens_val_to_name[level->sens - 1]; char *cats_str; - if (ebitmap_cardinality(cats) > 0) { + if (ebitmap_length(cats) > 0) { cats_str = cats_ebitmap_to_str(cats, pdb->p_cat_val_to_name); level_str = create_str("%s:%s", 2, sens_str, cats_str); free(cats_str); @@ -2145,7 +2145,7 @@ static int write_role_decl_rules_to_conf(FILE *out, struct policydb *pdb) rc = -1; goto exit; } - if (ebitmap_cardinality(&role->types.types) == 0) continue; + if (ebitmap_length(&role->types.types) == 0) continue; types = ebitmap_to_str(&role->types.types, pdb->p_type_val_to_name, 1); if (!types) { rc = -1; @@ -2298,7 +2298,7 @@ static int write_user_decl_rules_to_conf(FILE *out, struct policydb *pdb) } sepol_printf(out, "user %s", name); - if (ebitmap_cardinality(&user->roles.roles) > 0) { + if (ebitmap_length(&user->roles.roles) > 0) { roles = ebitmap_to_str(&user->roles.roles, pdb->p_role_val_to_name, 1); if (!roles) { diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c index e20c3d44..b1cbef08 100644 --- a/libsepol/src/module_to_cil.c +++ b/libsepol/src/module_to_cil.c @@ -2069,7 +2069,7 @@ static int class_order_to_cil(int indent, struct policydb *pdb, struct ebitmap o struct ebitmap_node *node; uint32_t i; - if (ebitmap_cardinality(&order) == 0) { + if (ebitmap_length(&order) == 0) { return 0; } @@ -2149,7 +2149,7 @@ static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN } } - if (ebitmap_cardinality(&role->dominates) > 1) { + if (ebitmap_length(&role->dominates) > 1) { log_err("Warning: role 'dominance' statement unsupported in CIL. Dropping from output."); } @@ -2175,7 +2175,7 @@ static int role_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN cil_println(indent, "(roleattribute %s)", key); } - if (ebitmap_cardinality(&role->roles) > 0) { + if (ebitmap_length(&role->roles) > 0) { cil_indent(indent); cil_printf("(roleattributeset %s (", key); ebitmap_for_each_positive_bit(&role->roles, node, i) { @@ -2269,7 +2269,7 @@ static int type_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN cil_printf(")\n"); } - if (ebitmap_cardinality(&type->types) > 0) { + if (ebitmap_length(&type->types) > 0) { cil_indent(indent); cil_printf("(typeattributeset %s (", key); ebitmap_to_cil(pdb, &type->types, SYM_TYPES); @@ -2372,7 +2372,7 @@ static int sens_to_cil(int indent, struct policydb *pdb, struct avrule_block *UN } } - if (ebitmap_cardinality(&level->level->cat) > 0) { + if (ebitmap_length(&level->level->cat) > 0) { cil_indent(indent); cil_printf("(sensitivitycategory %s (", key); ebitmap_to_cil(pdb, &level->level->cat, SYM_CATS); @@ -2387,7 +2387,7 @@ static int sens_order_to_cil(int indent, struct policydb *pdb, struct ebitmap or struct ebitmap_node *node; uint32_t i; - if (ebitmap_cardinality(&order) == 0) { + if (ebitmap_length(&order) == 0) { return 0; } @@ -2427,7 +2427,7 @@ static int cat_order_to_cil(int indent, struct policydb *pdb, struct ebitmap ord struct ebitmap_node *node; uint32_t i; - if (ebitmap_cardinality(&order) == 0) { + if (ebitmap_length(&order) == 0) { rc = 0; goto exit; } @@ -2478,7 +2478,7 @@ static int level_to_cil(struct policydb *pdb, struct mls_level *level) cil_printf("(%s", pdb->p_sens_val_to_name[level->sens - 1]); - if (ebitmap_cardinality(map) > 0) { + if (ebitmap_length(map) > 0) { cil_printf("("); ebitmap_to_cil(pdb, map, SYM_CATS); cil_printf(")");
When checking whether or not an ebitmap has any bits set, use ebitmap_length() instead of ebitmap_cardinality(). There is no need to find out how many bits are set, if all that is needed is to determine if any bits are set at all. Signed-off-by: James Carter <jwcart2@tycho.nsa.gov> --- libsepol/src/kernel_to_cil.c | 10 +++++----- libsepol/src/kernel_to_conf.c | 8 ++++---- libsepol/src/module_to_cil.c | 16 ++++++++-------- 3 files changed, 17 insertions(+), 17 deletions(-)