| Message ID | a85d28e8cbd32acda21424ab6a46089f26969671.camel@perches.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On 06/12/2018 04:38 PM, Joe Perches wrote: > On Tue, 2018-06-12 at 10:09 +0200, Peter Enderborg wrote: >> Replace printk with pr_* to avoid checkpatch warnings. > I believe it would be nicer to remove the > "SELinux: " prefix embbeded in each format > and use a specific > > #define pr_fmt(fmt) "SELinux: " fmt > > to automatically prefix these formats. I cant argument about that, however some of the warnings and debug prints in this set does not have this so it will then change the actual output. (And I also think that they should have a the prefix, but I don't know why they don't) So I am not sure if it appropriate for a cleanup patch, it supposed to have no functional change. >> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c > [] >> @@ -96,7 +96,7 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node) >> if (new_state != node->cur_state) { >> node->cur_state = new_state; >> if (new_state == -1) >> - printk(KERN_ERR "SELinux: expression result was undefined - disabling all rules.\n"); >> + pr_err("SELinux: expression result was undefined - disabling all rules.\n"); >> /* turn the rules on or off */ >> for (cur = node->true_list; cur; cur = cur->next) { >> if (new_state <= 0) > So, for instance, this patch could become: > (etc and so forth for each patch in this series) > > --- > security/selinux/ss/conditional.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c > index c91543a617ac..e96820d92b61 100644 > --- a/security/selinux/ss/conditional.c > +++ b/security/selinux/ss/conditional.c > @@ -7,6 +7,8 @@ > * the Free Software Foundation, version 2. > */ > > +#define pr_fmt(fmt) "SELinux: " fmt > + > #include <linux/kernel.h> > #include <linux/errno.h> > #include <linux/string.h> > @@ -96,7 +98,7 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node) > if (new_state != node->cur_state) { > node->cur_state = new_state; > if (new_state == -1) > - printk(KERN_ERR "SELinux: expression result was undefined - disabling all rules.\n"); > + pr_err("expression result was undefined - disabling all rules\n"); > /* turn the rules on or off */ > for (cur = node->true_list; cur; cur = cur->next) { > if (new_state <= 0) > @@ -287,7 +289,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum > */ > if (k->specified & AVTAB_TYPE) { > if (avtab_search(&p->te_avtab, k)) { > - printk(KERN_ERR "SELinux: type rule already exists outside of a conditional.\n"); > + pr_err("type rule already exists outside of a conditional\n"); > goto err; > } > /* > @@ -302,7 +304,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum > node_ptr = avtab_search_node(&p->te_cond_avtab, k); > if (node_ptr) { > if (avtab_search_node_next(node_ptr, k->specified)) { > - printk(KERN_ERR "SELinux: too many conflicting type rules.\n"); > + pr_err("too many conflicting type rules\n"); > goto err; > } > found = 0; > @@ -313,13 +315,13 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum > } > } > if (!found) { > - printk(KERN_ERR "SELinux: conflicting type rules.\n"); > + pr_err("conflicting type rules\n"); > goto err; > } > } > } else { > if (avtab_search(&p->te_cond_avtab, k)) { > - printk(KERN_ERR "SELinux: conflicting type rules when adding type rule for true.\n"); > + pr_err("conflicting type rules when adding type rule for true\n"); > goto err; > } > } > @@ -327,7 +329,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum > > node_ptr = avtab_insert_nonunique(&p->te_cond_avtab, k, d); > if (!node_ptr) { > - printk(KERN_ERR "SELinux: could not insert rule.\n"); > + pr_err("could not insert rule\n"); > rc = -ENOMEM; > goto err; > } > @@ -387,12 +389,12 @@ static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list * > static int expr_isvalid(struct policydb *p, struct cond_expr *expr) > { > if (expr->expr_type <= 0 || expr->expr_type > COND_LAST) { > - printk(KERN_ERR "SELinux: conditional expressions uses unknown operator.\n"); > + pr_err("conditional expressions uses unknown operator\n"); > return 0; > } > > if (expr->bool > p->p_bools.nprim) { > - printk(KERN_ERR "SELinux: conditional expressions uses unknown bool.\n"); > + pr_err("conditional expressions uses unknown bool\n"); > return 0; > } > return 1; > >
On 6/12/18 11:23 PM, peter enderborg wrote: > On 06/12/2018 04:38 PM, Joe Perches wrote: >> On Tue, 2018-06-12 at 10:09 +0200, Peter Enderborg wrote: >>> Replace printk with pr_* to avoid checkpatch warnings. >> I believe it would be nicer to remove the >> "SELinux: " prefix embbeded in each format >> and use a specific >> >> #define pr_fmt(fmt) "SELinux: " fmt >> >> to automatically prefix these formats. > I cant argument about that, however some of the warnings and debug prints in this set does not have this > so it will then change the actual output. (And I also think that they should have a the prefix, but I don't > know why they don't) So I am not sure if it appropriate for a cleanup patch, it supposed to have no functional change. I would suggest that could be a follow-up patch. I do like the cleanup, and it's better than the status quo. Acked-by: Jay Freyensee <why2jjj.linux@gmail.com> >>> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c >> [] >>> @@ -96,7 +96,7 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node) >>> if (new_state != node->cur_state) { >>> node->cur_state = new_state; >>> if (new_state == -1) >>> - printk(KERN_ERR "SELinux: expression result was undefined - disabling all rules.\n"); >>> + pr_err("SELinux: expression result was undefined - disabling all rules.\n"); >>> /* turn the rules on or off */ >>> for (cur = node->true_list; cur; cur = cur->next) { >>> if (new_state <= 0) >> So, for instance, this patch could become: >> (etc and so forth for each patch in this series) >> >> --- >> security/selinux/ss/conditional.c | 18 ++++++++++-------- >> 1 file changed, 10 insertions(+), 8 deletions(-) >> >> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c >> index c91543a617ac..e96820d92b61 100644 >> --- a/security/selinux/ss/conditional.c >> +++ b/security/selinux/ss/conditional.c >> @@ -7,6 +7,8 @@ >> * the Free Software Foundation, version 2. >> */ >> >> +#define pr_fmt(fmt) "SELinux: " fmt >> + >> #include <linux/kernel.h> >> #include <linux/errno.h> >> #include <linux/string.h> >> @@ -96,7 +98,7 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node) >> if (new_state != node->cur_state) { >> node->cur_state = new_state; >> if (new_state == -1) >> - printk(KERN_ERR "SELinux: expression result was undefined - disabling all rules.\n"); >> + pr_err("expression result was undefined - disabling all rules\n"); >> /* turn the rules on or off */ >> for (cur = node->true_list; cur; cur = cur->next) { >> if (new_state <= 0) >> @@ -287,7 +289,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum >> */ >> if (k->specified & AVTAB_TYPE) { >> if (avtab_search(&p->te_avtab, k)) { >> - printk(KERN_ERR "SELinux: type rule already exists outside of a conditional.\n"); >> + pr_err("type rule already exists outside of a conditional\n"); >> goto err; >> } >> /* >> @@ -302,7 +304,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum >> node_ptr = avtab_search_node(&p->te_cond_avtab, k); >> if (node_ptr) { >> if (avtab_search_node_next(node_ptr, k->specified)) { >> - printk(KERN_ERR "SELinux: too many conflicting type rules.\n"); >> + pr_err("too many conflicting type rules\n"); >> goto err; >> } >> found = 0; >> @@ -313,13 +315,13 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum >> } >> } >> if (!found) { >> - printk(KERN_ERR "SELinux: conflicting type rules.\n"); >> + pr_err("conflicting type rules\n"); >> goto err; >> } >> } >> } else { >> if (avtab_search(&p->te_cond_avtab, k)) { >> - printk(KERN_ERR "SELinux: conflicting type rules when adding type rule for true.\n"); >> + pr_err("conflicting type rules when adding type rule for true\n"); >> goto err; >> } >> } >> @@ -327,7 +329,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum >> >> node_ptr = avtab_insert_nonunique(&p->te_cond_avtab, k, d); >> if (!node_ptr) { >> - printk(KERN_ERR "SELinux: could not insert rule.\n"); >> + pr_err("could not insert rule\n"); >> rc = -ENOMEM; >> goto err; >> } >> @@ -387,12 +389,12 @@ static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list * >> static int expr_isvalid(struct policydb *p, struct cond_expr *expr) >> { >> if (expr->expr_type <= 0 || expr->expr_type > COND_LAST) { >> - printk(KERN_ERR "SELinux: conditional expressions uses unknown operator.\n"); >> + pr_err("conditional expressions uses unknown operator\n"); >> return 0; >> } >> >> if (expr->bool > p->p_bools.nprim) { >> - printk(KERN_ERR "SELinux: conditional expressions uses unknown bool.\n"); >> + pr_err("conditional expressions uses unknown bool\n"); >> return 0; >> } >> return 1; >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 13, 2018 at 2:23 AM peter enderborg <peter.enderborg@sony.com> wrote: > On 06/12/2018 04:38 PM, Joe Perches wrote: > > On Tue, 2018-06-12 at 10:09 +0200, Peter Enderborg wrote: > >> Replace printk with pr_* to avoid checkpatch warnings. > > I believe it would be nicer to remove the > > "SELinux: " prefix embbeded in each format > > and use a specific > > > > #define pr_fmt(fmt) "SELinux: " fmt > > > > to automatically prefix these formats. > > I cant argument about that, however some of the warnings and debug prints in this set does not have this > so it will then change the actual output. (And I also think that they should have a the prefix, but I don't > know why they don't) So I am not sure if it appropriate for a cleanup patch, it supposed to have no functional change. As others have mentioned, I think this patch is still a step forward so I'm going to go ahead and merge it; thanks Peter. As far as the prefix, or lack of, is concerned, that's probably an oversight that we should fix at some point, but we would need to look at each instance to verify.
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index c91543a617ac..e96820d92b61 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -7,6 +7,8 @@ * the Free Software Foundation, version 2. */ +#define pr_fmt(fmt) "SELinux: " fmt + #include <linux/kernel.h> #include <linux/errno.h> #include <linux/string.h> @@ -96,7 +98,7 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node) if (new_state != node->cur_state) { node->cur_state = new_state; if (new_state == -1) - printk(KERN_ERR "SELinux: expression result was undefined - disabling all rules.\n"); + pr_err("expression result was undefined - disabling all rules\n"); /* turn the rules on or off */ for (cur = node->true_list; cur; cur = cur->next) { if (new_state <= 0) @@ -287,7 +289,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum */ if (k->specified & AVTAB_TYPE) { if (avtab_search(&p->te_avtab, k)) { - printk(KERN_ERR "SELinux: type rule already exists outside of a conditional.\n"); + pr_err("type rule already exists outside of a conditional\n"); goto err; } /* @@ -302,7 +304,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum node_ptr = avtab_search_node(&p->te_cond_avtab, k); if (node_ptr) { if (avtab_search_node_next(node_ptr, k->specified)) { - printk(KERN_ERR "SELinux: too many conflicting type rules.\n"); + pr_err("too many conflicting type rules\n"); goto err; } found = 0; @@ -313,13 +315,13 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum } } if (!found) { - printk(KERN_ERR "SELinux: conflicting type rules.\n"); + pr_err("conflicting type rules\n"); goto err; } } } else { if (avtab_search(&p->te_cond_avtab, k)) { - printk(KERN_ERR "SELinux: conflicting type rules when adding type rule for true.\n"); + pr_err("conflicting type rules when adding type rule for true\n"); goto err; } } @@ -327,7 +329,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum node_ptr = avtab_insert_nonunique(&p->te_cond_avtab, k, d); if (!node_ptr) { - printk(KERN_ERR "SELinux: could not insert rule.\n"); + pr_err("could not insert rule\n"); rc = -ENOMEM; goto err; } @@ -387,12 +389,12 @@ static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list * static int expr_isvalid(struct policydb *p, struct cond_expr *expr) { if (expr->expr_type <= 0 || expr->expr_type > COND_LAST) { - printk(KERN_ERR "SELinux: conditional expressions uses unknown operator.\n"); + pr_err("conditional expressions uses unknown operator\n"); return 0; } if (expr->bool > p->p_bools.nprim) { - printk(KERN_ERR "SELinux: conditional expressions uses unknown bool.\n"); + pr_err("conditional expressions uses unknown bool\n"); return 0; } return 1;