[PATCH] errormsg: use descriptive macros for error numbers

[PATCH] errormsg: use descriptive macros for error numbers

[Richard Guy Briggs]

Convert all the numerical error return codes in comparison option and field
option parsing routines audit_rule_interfield_comp_data() and
audit_rule_fieldpair_data() to descriptive macros for easier code navigation
and verification.

See: https://github.com/linux-audit/audit-userspace/issues/11

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 lib/errormsg.h |   29 +++++++++++++++++++
 lib/libaudit.c |   84 ++++++++++++++++++++++++++++----------------------------
 2 files changed, 71 insertions(+), 42 deletions(-)

diff --git a/lib/errormsg.h b/lib/errormsg.h
index 9524697..17ff767 100644
--- a/lib/errormsg.h
+++ b/lib/errormsg.h
@@ -68,4 +68,33 @@ static const struct msg_tab err_msgtab[] = {
     { -30,    2,    "Field option not supported by kernel:" },
     { -31,    1,    "must be used with exclude, user, or exit filter" },
 };
+#define EAU_OPMISSING		1
+#define EAU_FIELDUNKNOWN	2
+#define EAU_ARCHMISPLACED	3
+#define EAU_ARCHUNKNOWN		4
+#define EAU_ELFUNKNOWN		5
+#define EAU_ARCHNOBIT		6
+#define EAU_EXITONLY		7
+#define EAU_MSGTYPEUNKNOWN	8
+#define EAU_MSGTYPEEXCLUDE	9
+#define EAU_UPGRADEFAIL		10
+#define EAU_STRTOOLONG		11
+#define EAU_MSGTYPECREDEXCLUDE	12
+#define EAU_OPEQNOTEQ		13
+#define EAU_PERMRWXA		14
+#define EAU_ERRUNKNOWN		15
+#define EAU_FILETYPEUNKNOWN	16
+#define EAU_EXITENTRYONLY	17
+#define EAU_KEYDEP		19
+#define EAU_FIELDVALMISSING	20
+#define EAU_FIELDVALNUM		21
+#define EAU_FIELDNAME		22
+#define EAU_COMPFIELDNAME	24
+#define EAU_COMPVAL		25
+#define EAU_COMPFIELDUNKNOWN	26
+#define EAU_COMPVALUNKNOWN	27
+#define EAU_FIELDTOOMANY	28
+#define EAU_OPEQ		29
+#define EAU_FIELDNOSUPPORT	30
+#define EAU_FIELDNOFILTER	31
 #endif
diff --git a/lib/libaudit.c b/lib/libaudit.c
index 002dd89..a3b4261 100644
--- a/lib/libaudit.c
+++ b/lib/libaudit.c
@@ -520,7 +520,7 @@ int audit_reset_lost(int fd)
 	struct audit_status s;
 
 	if ((audit_get_features() & AUDIT_FEATURE_BITMAP_LOST_RESET) == 0)
-		return -30;
+		return -EAU_FIELDNOSUPPORT;
 
 	memset(&s, 0, sizeof(s));
 	s.mask = AUDIT_STATUS_LOST;
@@ -979,7 +979,7 @@ int audit_rule_interfield_comp_data(struct audit_rule_data **rulep,
 		return -1;
 
 	if (rule->field_count >= (AUDIT_MAX_FIELDS - 1))
-		return -28;
+		return -EAU_FIELDTOOMANY;
 
 	/* look for 2-char operators first
 	   then look for 1-char operators afterwards
@@ -994,24 +994,24 @@ int audit_rule_interfield_comp_data(struct audit_rule_data **rulep,
 		*v++ = '\0';
 		op = AUDIT_EQUAL;
 	} else {
-		return -13;
+		return -EAU_OPEQNOTEQ;
 	}
 
 	if (*f == 0)
-		return -24;
+		return -EAU_COMPFIELDNAME;
 
 	if (*v == 0)
-		return -25;
+		return -EAU_COMPVAL;
 
 	if ((field1 = audit_name_to_field(f)) < 0)
-		return -26;
+		return -EAU_COMPFIELDUNKNOWN;
 
 	if ((field2 = audit_name_to_field(v)) < 0)
-		return -27;
+		return -EAU_COMPVALUNKNOWN;
 
 	/* Interfield comparison can only be in exit filter */
 	if (flags != AUDIT_FILTER_EXIT)
-		return -7;
+		return -EAU_EXITONLY;
 
 	// It should always be AUDIT_FIELD_COMPARE
 	rule->fields[rule->field_count] = AUDIT_FIELD_COMPARE;
@@ -1392,7 +1392,7 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 		return -1;
 
 	if (rule->field_count >= (AUDIT_MAX_FIELDS - 1))
-		return -28;
+		return -EAU_FIELDTOOMANY;
 
 	/* look for 2-char operators first
 	   then look for 1-char operators afterwards
@@ -1430,23 +1430,23 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 	}
 
 	if (v == NULL)
-		return -1;
+		return -EAU_OPMISSING;
 	
 	if (*f == 0)
-		return -22;
+		return -EAU_FIELDNAME;
 
 	if (*v == 0)
-		return -20;
+		return -EAU_FIELDVALMISSING;
 
 	if ((field = audit_name_to_field(f)) < 0) 
-		return -2;
+		return -EAU_FIELDUNKNOWN;
 
 	/* Exclude filter can be used only with MSGTYPE and cred fields */
 	if (flags == AUDIT_FILTER_EXCLUDE) {
 		uint32_t features = audit_get_features();
 		if ((features & AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND) == 0) {
 			if (field != AUDIT_MSGTYPE)
-				return -30;
+				return -EAU_FIELDNOSUPPORT;
 		} else {
 			switch(field) {
 				case AUDIT_PID:
@@ -1461,7 +1461,7 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 				case AUDIT_SUBJ_CLR:
 					break;
 				default:
-					return -12;
+					return -EAU_MSGTYPECREDEXCLUDE;
 			}
 		}
 	}
@@ -1529,13 +1529,13 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 				rule->values[rule->field_count] = 
 						audit_name_to_errno(v);
 				if (rule->values[rule->field_count] == 0) 
-					return -15;
+					return -EAU_ERRUNKNOWN;
 			}
 			break;
 		case AUDIT_MSGTYPE:
 			if (flags != AUDIT_FILTER_EXCLUDE &&
 					flags != AUDIT_FILTER_USER)
-				return -9;
+				return -EAU_MSGTYPEEXCLUDE;
 
 			if (isdigit((char)*(v)))
 				rule->values[rule->field_count] =
@@ -1545,7 +1545,7 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 					rule->values[rule->field_count] =
 						audit_name_to_msg_type(v);
 				else
-					return -8;
+					return -EAU_MSGTYPEUNKNOWN;
 			break;
 		/* These next few are strings */
 		case AUDIT_OBJ_USER:
@@ -1558,7 +1558,7 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 			/* Watch & object filtering is invalid on anything
 			 * but exit */
 			if (flags != AUDIT_FILTER_EXIT)
-				return -7;
+				return -EAU_EXITONLY;
 			if (field == AUDIT_WATCH || field == AUDIT_DIR)
 				_audit_permadded = 1;
 
@@ -1573,21 +1573,21 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 			if (field == AUDIT_EXE) {
 				uint32_t features = audit_get_features();
 				if ((features & AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH) == 0)
-					return -30;
+					return -EAU_FIELDNOSUPPORT;
 				if (op != AUDIT_EQUAL)
-					return -29;
+					return -EAU_OPEQ;
 				_audit_exeadded = 1;
 			}
 			if (field == AUDIT_FILTERKEY &&
 				!(_audit_syscalladded || _audit_permadded ||
 				_audit_exeadded))
-                                return -19;
+                                return -EAU_KEYDEP;
 			vlen = strlen(v);
 			if (field == AUDIT_FILTERKEY &&
 					vlen > AUDIT_MAX_KEY_LEN)
-				return -11;
+				return -EAU_STRTOOLONG;
 			else if (vlen > PATH_MAX)
-				return -11;
+				return -EAU_STRTOOLONG;
 			rule->values[rule->field_count] = vlen;
 			offset = rule->buflen;
 			rule->buflen += vlen;
@@ -1604,21 +1604,21 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 			break;
 		case AUDIT_ARCH:
 			if (_audit_syscalladded) 
-				return -3;
+				return -EAU_ARCHMISPLACED;
 			if (!(op == AUDIT_NOT_EQUAL || op == AUDIT_EQUAL))
-				return -13;
+				return -EAU_OPEQNOTEQ;
 			if (isdigit((char)*(v))) {
 				int machine;
 
 				errno = 0;
 				_audit_elf = strtoul(v, NULL, 0);
 				if (errno) 
-					return -5;
+					return -EAU_ELFUNKNOWN;
 
 				// Make sure we have a valid mapping
 				machine = audit_elf_to_machine(_audit_elf);
 				if (machine < 0)
-					return -5;
+					return -EAU_ELFUNKNOWN;
 			}
 			else {
 				const char *arch=v;
@@ -1628,7 +1628,7 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 				   to elf. */
 				elf = audit_machine_to_elf(machine);
 				if (elf == 0)
-					return -5;
+					return -EAU_ELFUNKNOWN;
 
 				_audit_elf = elf;
 			}
@@ -1637,15 +1637,15 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 			break;
 		case AUDIT_PERM:
 			if (flags != AUDIT_FILTER_EXIT)
-				return -7;
+				return -EAU_EXITONLY;
 			else if (op != AUDIT_EQUAL)
-				return -13;
+				return -EAU_OPEQNOTEQ;
 			else {
 				unsigned int i, len, val = 0;
 
 				len = strlen(v);
 				if (len > 4)
-					return -11;
+					return -EAU_STRTOOLONG;
 
 				for (i = 0; i < len; i++) {
 					switch (tolower(v[i])) {
@@ -1662,7 +1662,7 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 							val |= AUDIT_PERM_ATTR;
 							break;
 						default:
-							return -14;
+							return -EAU_PERMRWXA;
 					}
 				}
 				rule->values[rule->field_count] = val;
@@ -1670,11 +1670,11 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 			break;
 		case AUDIT_FILETYPE:
 			if (!(flags == AUDIT_FILTER_EXIT))
-				return -17;
+				return -EAU_EXITENTRYONLY;
 			rule->values[rule->field_count] = 
 				audit_name_to_ftype(v);
 			if ((int)rule->values[rule->field_count] < 0) {
-				return -16;
+				return -EAU_FILETYPEUNKNOWN;
 			}
 			break;
 		case AUDIT_ARG0...AUDIT_ARG3:
@@ -1687,16 +1687,16 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 				rule->values[rule->field_count] =
 					strtol(v, NULL, 0);
 			else 
-				return -21;
+				return -EAU_FIELDVALNUM;
 			break;
 		case AUDIT_SESSIONID:
 			if ((audit_get_features() &
 				AUDIT_FEATURE_BITMAP_SESSIONID_FILTER) == 0)
-				return -30;
+				return -EAU_FIELDNOSUPPORT;
 			if (flags != AUDIT_FILTER_EXCLUDE &&
 			    flags != AUDIT_FILTER_USER &&
 			    flags != AUDIT_FILTER_EXIT)
-				return -31;
+				return -EAU_FIELDNOFILTER;
 			// Do positive & negative separate for 32 bit systems
 			vlen = strlen(v);
 			if (isdigit((char)*(v))) 
@@ -1712,20 +1712,20 @@ int audit_rule_fieldpair_data(struct audit_rule_data **rulep, const char *pair,
 		case AUDIT_DEVMAJOR...AUDIT_INODE:
 		case AUDIT_SUCCESS:
 			if (flags != AUDIT_FILTER_EXIT)
-				return -7;
+				return -EAU_EXITONLY;
 			/* fallthrough */
 		default:
 			if (field == AUDIT_INODE) {
 				if (!(op == AUDIT_NOT_EQUAL ||
 							op == AUDIT_EQUAL))
-					return -13;
+					return -EAU_OPEQNOTEQ;
 			}
 
 			if (field == AUDIT_PPID && !(flags==AUDIT_FILTER_EXIT))
-				return -17;
+				return -EAU_EXITENTRYONLY;
 			
 			if (!isdigit((char)*(v)))
-				return -21;
+				return -EAU_FIELDVALNUM;
 
 			if (field == AUDIT_INODE)
 				rule->values[rule->field_count] =
-- 
1.7.1

Re: [PATCH] errormsg: use descriptive macros for error numbers

[Steve Grubb]

On Tuesday, April 4, 2017 6:36:52 AM EDT Richard Guy Briggs wrote:
> Convert all the numerical error return codes in comparison option and field
> option parsing routines audit_rule_interfield_comp_data() and
> audit_rule_fieldpair_data() to descriptive macros for easier code navigation
> and verification.
> 
> See: https://github.com/linux-audit/audit-userspace/issues/11
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  lib/errormsg.h |   29 +++++++++++++++++++
>  lib/libaudit.c |   84
> ++++++++++++++++++++++++++++---------------------------- 2 files changed,
> 71 insertions(+), 42 deletions(-)

Applied. But we still have hardcoded numbers in the err_msgtab[].  :-)

Thanks,
-Steve

Re: [PATCH] errormsg: use descriptive macros for error numbers

[Richard Guy Briggs]

On 2017-04-11 17:33, Steve Grubb wrote:
> On Tuesday, April 4, 2017 6:36:52 AM EDT Richard Guy Briggs wrote:
> > Convert all the numerical error return codes in comparison option and field
> > option parsing routines audit_rule_interfield_comp_data() and
> > audit_rule_fieldpair_data() to descriptive macros for easier code navigation
> > and verification.
> > 
> > See: https://github.com/linux-audit/audit-userspace/issues/11
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  lib/errormsg.h |   29 +++++++++++++++++++
> >  lib/libaudit.c |   84
> > ++++++++++++++++++++++++++++---------------------------- 2 files changed,
> > 71 insertions(+), 42 deletions(-)
> 
> Applied.

Thanks for taking this patch.  Are you able to adapt your workflow so
that the original author of contributed patches shows up in the git
commit author line with the original patch timestamp?  Looking back
through the repo history it appears you are the only author since you
took over from mitr, which I know not to be true.  As it is now, you
appear to be the author of this patch and the patch date is a week later
than it actually was.  There is no way to the original author now.  "git
am" is able to do this.

> But we still have hardcoded numbers in the err_msgtab[].  :-)

Yes.  Once all the return codes that use this function have been
accounted for and have been converted to macros, we can throw the macros
into an enum and not care what their actual value is and then convert
the table.  One step at a time!  I wasn't able to see any of the
duplication, overloading or message meanings drifting over time without
this first step.


Note: GitHub Milestones: I had arbitrarily picked the only listed
milestone in github (which seemed reasonable at the time) to see how
well that mechanism worked.  Was that an appropriate milestone?  Can we
create some more and start using that facility for items in your TODO?


> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH] errormsg: use descriptive macros for error numbers

[Steve Grubb]

On Wednesday, April 12, 2017 2:39:26 AM EDT Richard Guy Briggs wrote:
> On 2017-04-11 17:33, Steve Grubb wrote:
> > On Tuesday, April 4, 2017 6:36:52 AM EDT Richard Guy Briggs wrote:
> > > Convert all the numerical error return codes in comparison option and
> > > field option parsing routines audit_rule_interfield_comp_data() and
> > > audit_rule_fieldpair_data() to descriptive macros for easier code
> > > navigation and verification.
> > > 
> > > See: https://github.com/linux-audit/audit-userspace/issues/11
> > > 
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > 
> > >  lib/errormsg.h |   29 +++++++++++++++++++
> > >  lib/libaudit.c |   84
> > > 
> > > ++++++++++++++++++++++++++++---------------------------- 2 files
> > > changed,
> > > 71 insertions(+), 42 deletions(-)
> > 
> > Applied.
> 
> Thanks for taking this patch.  Are you able to adapt your workflow so
> that the original author of contributed patches shows up in the git
> commit author line with the original patch timestamp?

Not sure how that works. patch -p1 < email.mbox doesn't really allow for that.

> Looking back through the repo history it appears you are the only author
> since you took over from mitr, which I know not to be true.

I give attribution in the Changelog when the patch fixes a bug or adds a 
feature. Spelling errors and shuffling code around usually don't get any 
mention. If there is a big contribution, I put something in the THANKS file. 
This project has lasted longer that git or github has been around. I'm 
following the GNITS standard.

> As it is now, you appear to be the author of this patch and the patch date
> is a week later than it actually was.  There is no way to the original
> author now.  "git am" is able to do this.

Is this really a problem? I have sent hundreds of lines of fixes to shadow-
utils and I honestly don't care what's in their git logs or even care if they 
use git. This project has always used the Changelog and THANKS files for 
attribution. And they survive moving to another SCCS if we decide to do that.

> > But we still have hardcoded numbers in the err_msgtab[].  :-)
> 
> Yes.  Once all the return codes that use this function have been
> accounted for and have been converted to macros, we can throw the macros
> into an enum and not care what their actual value is and then convert
> the table.

Could have done that all in one shot. That would have been preferred. But 
don't take this the wrong way. Thanks for your contribution.

> One step at a time!  I wasn't able to see any of the duplication,
> overloading or message meanings drifting over time without
> this first step.
> 
> 
> Note: GitHub Milestones: I had arbitrarily picked the only listed
> milestone in github (which seemed reasonable at the time) to see how
> well that mechanism worked.  Was that an appropriate milestone? 

I have not looked. I do not use the milestones. Things happen. Plans change.

> Can we create some more and start using that facility for items in your
> TODO?

I don't want to make more work for myself. No one contributes anything that is 
in the TODO except one time. (I have to give a shout out to Burn for tackling 
the auparse lol conversion. That was a major step in reliability for auparse.) 
But seriously, why should I make things harder on myself when there is not 
much in the way of help? I also review and reprioritize things every now and 
then. Having milestones would just make that messier. For example, two weeks 
ago I did not know we'd be doing a 2.7.5 release this week.

If people really would like to contribute, the most important thing right now 
is cleaning up the events. The log normalizer makes this a much higher 
priority because it shows exactly when events are needing to be fixed. No 
milestones are needed. Patches are needed.  :-)

And if anyone wants to contribute user space code, just ask on the list or 
privately how you can help. I can pick something from the TODO that can be 
simple or a medium sized chunk. I also expect a little discussion before doing 
it to make sure the proposed patch is close to how I envisioned it.

-Steve

Re: [PATCH] errormsg: use descriptive macros for error numbers

[Richard Guy Briggs]

On 2017-04-12 18:14, Steve Grubb wrote:
> On Wednesday, April 12, 2017 2:39:26 AM EDT Richard Guy Briggs wrote:
> > On 2017-04-11 17:33, Steve Grubb wrote:
> > > On Tuesday, April 4, 2017 6:36:52 AM EDT Richard Guy Briggs wrote:
> > > > Convert all the numerical error return codes in comparison option and
> > > > field option parsing routines audit_rule_interfield_comp_data() and
> > > > audit_rule_fieldpair_data() to descriptive macros for easier code
> > > > navigation and verification.
> > > > 
> > > > See: https://github.com/linux-audit/audit-userspace/issues/11
> > > > 
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > 
> > > >  lib/errormsg.h |   29 +++++++++++++++++++
> > > >  lib/libaudit.c |   84
> > > > 
> > > > ++++++++++++++++++++++++++++---------------------------- 2 files
> > > > changed,
> > > > 71 insertions(+), 42 deletions(-)
> > > 
> > > Applied.
> > 
> > Thanks for taking this patch.  Are you able to adapt your workflow so
> > that the original author of contributed patches shows up in the git
> > commit author line with the original patch timestamp?
> 
> Not sure how that works. patch -p1 < email.mbox doesn't really allow for that.

"git am -s <mbox>|<Maildir>" will do that and add your signature.

> > Looking back through the repo history it appears you are the only author
> > since you took over from mitr, which I know not to be true.
> 
> I give attribution in the Changelog when the patch fixes a bug or adds a 
> feature. Spelling errors and shuffling code around usually don't get any 
> mention. If there is a big contribution, I put something in the THANKS file. 
> This project has lasted longer that git or github has been around. I'm 
> following the GNITS standard.

If you use git am instead, it'll be self-documenting.  I seem to recall
svn had similar facilities.

Linus has been doing this since v2.5.3 in 2002 through at least one SCM
system change.  I'm pretty sure he had that history before that too for
GPL copyright licence accountability, but it was too painful to port to
the new SCM adopted in 2002.  The recent legal copyright licence query
about a few files in the audit userspace package would have been easier
to answer with that automation.

> > As it is now, you appear to be the author of this patch and the patch date
> > is a week later than it actually was.  There is no way to the original
> > author now.  "git am" is able to do this.
> 
> Is this really a problem? I have sent hundreds of lines of fixes to shadow-
> utils and I honestly don't care what's in their git logs or even care if they 
> use git. This project has always used the Changelog and THANKS files for 
> attribution. And they survive moving to another SCCS if we decide to do that.

It is when people are looking for automated statistics.  I see no need
to change the use of the Changelog and THANKS files.

> > > But we still have hardcoded numbers in the err_msgtab[].  :-)
> > 
> > Yes.  Once all the return codes that use this function have been
> > accounted for and have been converted to macros, we can throw the macros
> > into an enum and not care what their actual value is and then convert
> > the table.
> 
> Could have done that all in one shot. That would have been preferred. But 
> don't take this the wrong way. Thanks for your contribution.

There are still two more patches outstanding that were part of that
effort.  I split them up because the first step was more obvious and
made the other steps easier to understand.  I could have squashed all
three patches together, but I chose to leave them seperate to make them
easier to understand when being reviewed and accpted.  I figured there
was a greater chance of the three patches being accepted if it was clear
what each one did.  The other two have not yet been accepted, so that
suggests to me that they required more careful review since they changed
existing behaviour as opposed to this one that did not.

> > One step at a time!  I wasn't able to see any of the duplication,
> > overloading or message meanings drifting over time without
> > this first step.
> > 
> > 
> > Note: GitHub Milestones: I had arbitrarily picked the only listed
> > milestone in github (which seemed reasonable at the time) to see how
> > well that mechanism worked.  Was that an appropriate milestone? 
> 
> I have not looked. I do not use the milestones. Things happen. Plans change.

So who created that one existing milestone?

> > Can we create some more and start using that facility for items in your
> > TODO?
> 
> I don't want to make more work for myself. No one contributes anything that is 
> in the TODO except one time. (I have to give a shout out to Burn for tackling 
> the auparse lol conversion. That was a major step in reliability for auparse.) 
> But seriously, why should I make things harder on myself when there is not 
> much in the way of help? I also review and reprioritize things every now and 
> then. Having milestones would just make that messier. For example, two weeks 
> ago I did not know we'd be doing a 2.7.5 release this week.

It might just make things easier for others to participate.

> If people really would like to contribute, the most important thing right now 
> is cleaning up the events. The log normalizer makes this a much higher 
> priority because it shows exactly when events are needing to be fixed. No 
> milestones are needed. Patches are needed.  :-)

If there is a kernel github issue for each of the records that need
cleanup, they are on the list.

> And if anyone wants to contribute user space code, just ask on the list or 
> privately how you can help. I can pick something from the TODO that can be 
> simple or a medium sized chunk. I also expect a little discussion before doing 
> it to make sure the proposed patch is close to how I envisioned it.

It sounds so simple, but not everybody is comfortable asking how they
can help and just dive in to see how things work and start fixing or
building things.

> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH] errormsg: use descriptive macros for error numbers

[Richard Guy Briggs]

On 2017-04-13 04:42, Richard Guy Briggs wrote:
> On 2017-04-12 18:14, Steve Grubb wrote:
> > On Wednesday, April 12, 2017 2:39:26 AM EDT Richard Guy Briggs wrote:
> > > On 2017-04-11 17:33, Steve Grubb wrote:
> > > > On Tuesday, April 4, 2017 6:36:52 AM EDT Richard Guy Briggs wrote:
> > > > > Convert all the numerical error return codes in comparison option and
> > > > > field option parsing routines audit_rule_interfield_comp_data() and
> > > > > audit_rule_fieldpair_data() to descriptive macros for easier code
> > > > > navigation and verification.
> > > > > 
> > > > > See: https://github.com/linux-audit/audit-userspace/issues/11
> > > > > 
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > > 
> > > > >  lib/errormsg.h |   29 +++++++++++++++++++
> > > > >  lib/libaudit.c |   84
> > > > > 
> > > > > ++++++++++++++++++++++++++++---------------------------- 2 files
> > > > > changed,
> > > > > 71 insertions(+), 42 deletions(-)
> > > > 
> > > > Applied.
> > > 
> > > Thanks for taking this patch.  Are you able to adapt your workflow so
> > > that the original author of contributed patches shows up in the git
> > > commit author line with the original patch timestamp?
> > 
> > Not sure how that works. patch -p1 < email.mbox doesn't really allow for that.
> 
> "git am -s <mbox>|<Maildir>" will do that and add your signature.

I should add, "git am -s --reject <mbox>" will apply the hunks it can
and leave you with .rej files that make it much easier to deal with
patches that don't apply cleanly rather than just saying it failed
(though I don't need to tell you that you would be in your rights to
simply reject the patch if it doesn't apply cleanly, requesting the
submitter to use an updated tree).

> > > Looking back through the repo history it appears you are the only author
> > > since you took over from mitr, which I know not to be true.
> > 
> > I give attribution in the Changelog when the patch fixes a bug or adds a 
> > feature. Spelling errors and shuffling code around usually don't get any 
> > mention. If there is a big contribution, I put something in the THANKS file. 
> > This project has lasted longer that git or github has been around. I'm 
> > following the GNITS standard.
> 
> If you use git am instead, it'll be self-documenting.  I seem to recall
> svn had similar facilities.
> 
> Linus has been doing this since v2.5.3 in 2002 through at least one SCM
> system change.  I'm pretty sure he had that history before that too for
> GPL copyright licence accountability, but it was too painful to port to
> the new SCM adopted in 2002.  The recent legal copyright licence query
> about a few files in the audit userspace package would have been easier
> to answer with that automation.
> 
> > > As it is now, you appear to be the author of this patch and the patch date
> > > is a week later than it actually was.  There is no way to the original
> > > author now.  "git am" is able to do this.
> > 
> > Is this really a problem? I have sent hundreds of lines of fixes to shadow-
> > utils and I honestly don't care what's in their git logs or even care if they 
> > use git. This project has always used the Changelog and THANKS files for 
> > attribution. And they survive moving to another SCCS if we decide to do that.
> 
> It is when people are looking for automated statistics.  I see no need
> to change the use of the Changelog and THANKS files.
> 
> > > > But we still have hardcoded numbers in the err_msgtab[].  :-)
> > > 
> > > Yes.  Once all the return codes that use this function have been
> > > accounted for and have been converted to macros, we can throw the macros
> > > into an enum and not care what their actual value is and then convert
> > > the table.
> > 
> > Could have done that all in one shot. That would have been preferred. But 
> > don't take this the wrong way. Thanks for your contribution.
> 
> There are still two more patches outstanding that were part of that
> effort.  I split them up because the first step was more obvious and
> made the other steps easier to understand.  I could have squashed all
> three patches together, but I chose to leave them seperate to make them
> easier to understand when being reviewed and accpted.  I figured there
> was a greater chance of the three patches being accepted if it was clear
> what each one did.  The other two have not yet been accepted, so that
> suggests to me that they required more careful review since they changed
> existing behaviour as opposed to this one that did not.
> 
> > > One step at a time!  I wasn't able to see any of the duplication,
> > > overloading or message meanings drifting over time without
> > > this first step.
> > > 
> > > 
> > > Note: GitHub Milestones: I had arbitrarily picked the only listed
> > > milestone in github (which seemed reasonable at the time) to see how
> > > well that mechanism worked.  Was that an appropriate milestone? 
> > 
> > I have not looked. I do not use the milestones. Things happen. Plans change.
> 
> So who created that one existing milestone?
> 
> > > Can we create some more and start using that facility for items in your
> > > TODO?
> > 
> > I don't want to make more work for myself. No one contributes anything that is 
> > in the TODO except one time. (I have to give a shout out to Burn for tackling 
> > the auparse lol conversion. That was a major step in reliability for auparse.) 
> > But seriously, why should I make things harder on myself when there is not 
> > much in the way of help? I also review and reprioritize things every now and 
> > then. Having milestones would just make that messier. For example, two weeks 
> > ago I did not know we'd be doing a 2.7.5 release this week.
> 
> It might just make things easier for others to participate.
> 
> > If people really would like to contribute, the most important thing right now 
> > is cleaning up the events. The log normalizer makes this a much higher 
> > priority because it shows exactly when events are needing to be fixed. No 
> > milestones are needed. Patches are needed.  :-)
> 
> If there is a kernel github issue for each of the records that need
> cleanup, they are on the list.
> 
> > And if anyone wants to contribute user space code, just ask on the list or 
> > privately how you can help. I can pick something from the TODO that can be 
> > simple or a medium sized chunk. I also expect a little discussion before doing 
> > it to make sure the proposed patch is close to how I envisioned it.
> 
> It sounds so simple, but not everybody is comfortable asking how they
> can help and just dive in to see how things work and start fixing or
> building things.
> 
> > -Steve
> 
> - RGB
> 
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635