Refactoring src/ausearch-report.c:output_interpreted_node()

Refactoring src/ausearch-report.c:output_interpreted_node()

[Burn Alting]

Steve,

In lib/lookup_table.c:audit_name_to_msg_type(), the event type value is
parsed and converted to an integer as per,

Given
        type=<type_value> 
then
        <type_value>
is parsed for
        - a known string 
        - a long integer number, n, found in the specific string
		"UNKNOWN[n]"
        - a long integer number, n, found in the specific string
		"n"

In src/ausearch-report.c:output_interpreted_node() it additionally
parses for a <type_value> of
        - a long integer number, n, found in the string "[^\[]*[n].*"
i.e.
        type=something[n]something_else

Is there any reason against adding this additional parsing into
lib/lookup_table.c:audit_name_to_msg_type()?

If we can, then output_interpreted_node() can be re-factored so we are
not parsing the same data twice for every event.

I am uncertain what effect of accepting this additional format would
have when adding rules to the running audit system - i.e.
audit_name_to_msg_type() is called by autrace/auditctl when parsing
rules (ie the msgtype field name).


Regards

Burn

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Burn Alting]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

And here is the patch that updates audit_name_to_msg_type()


Rgds

On Mon, 2014-09-29 at 12:41 +1000, Burn Alting wrote:
> Steve,
> 
> In lib/lookup_table.c:audit_name_to_msg_type(), the event type value is
> parsed and converted to an integer as per,
> 
> Given
>         type=<type_value> 
> then
>         <type_value>
> is parsed for
>         - a known string 
>         - a long integer number, n, found in the specific string
> 		"UNKNOWN[n]"
>         - a long integer number, n, found in the specific string
> 		"n"
> 
> In src/ausearch-report.c:output_interpreted_node() it additionally
> parses for a <type_value> of
>         - a long integer number, n, found in the string "[^\[]*[n].*"
> i.e.
>         type=something[n]something_else
> 
> Is there any reason against adding this additional parsing into
> lib/lookup_table.c:audit_name_to_msg_type()?
> 
> If we can, then output_interpreted_node() can be re-factored so we are
> not parsing the same data twice for every event.
> 
> I am uncertain what effect of accepting this additional format would
> have when adding rules to the running audit system - i.e.
> audit_name_to_msg_type() is called by autrace/auditctl when parsing
> rules (ie the msgtype field name).
> 
> 
> Regards
> 
> Burn
> 
> 
> 
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit


[-- Attachment #2: audit-2.4_type_parsing.patch --]
[-- Type: text/x-patch, Size: 1188 bytes --]

diff -Npru audit-2.4/lib/lookup_table.c audit-2.4_type_parsing/lib/lookup_table.c
--- audit-2.4/lib/lookup_table.c	2014-08-25 02:39:27.000000000 +1000
+++ audit-2.4_type_parsing/lib/lookup_table.c	2014-09-29 12:54:22.555781561 +1000
@@ -224,23 +224,34 @@ const char *audit_action_to_name(int act
 int audit_name_to_msg_type(const char *msg_type)
 {
 	int rc;
+	char * bptr;
 
 	if (msg_type_s2i(msg_type, &rc) != 0)
 		return rc;
 
 	/* Take a stab at converting */
-	if (strncmp(msg_type, "UNKNOWN[", 8) == 0) {
+	if ((bptr = strchr(msg_type, '['))) {
 		int len;
 		char buf[8];
-		const char *end = strchr(msg_type + 8, ']');
+		const char *end;
+		/*
+ 		 * First check for "type=UNKNOWN[", otherwise
+ 		 * we accept anything before the '['
+ 		 */
+		if (strncmp(msg_type, "UNKNOWN[", 8) == 0) {
+			bptr = (char *)msg_type + 8;
+		} else {
+			bptr++;
+		}
+		end = strchr(bptr, ']');
 		if (end == NULL)
 			return -1;
 
-		len = end - (msg_type + 8);
+		len = end - bptr;
 		if (len > 7)
 			len = 7;
 		memset(buf, 0, sizeof(buf));
-		strncpy(buf, msg_type + 8, len);
+		strncpy(buf, bptr, len);
 		errno = 0;
 		return strtol(buf, NULL, 10);
 	} else if (isdigit(*msg_type)) {

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Steve Grubb]

On Monday, September 29, 2014 12:41:23 PM Burn Alting wrote:
> In lib/lookup_table.c:audit_name_to_msg_type(), the event type value is
> parsed and converted to an integer as per,
> 
> Given
>         type=<type_value>
> then
>         <type_value>
> is parsed for
>         - a known string
>         - a long integer number, n, found in the specific string
> 		"UNKNOWN[n]"
>         - a long integer number, n, found in the specific string
> 		"n"
> 
> In src/ausearch-report.c:output_interpreted_node() it additionally
> parses for a <type_value> of
>         - a long integer number, n, found in the string "[^\[]*[n].*"
> i.e.
>         type=something[n]something_else

This is specifically a fixup for the UNKNOWN[####] case. There is no other value 
it can be. This originates here:

https://fedorahosted.org/audit/browser/trunk/src/auditd-event.c#L1054


> Is there any reason against adding this additional parsing into
> lib/lookup_table.c:audit_name_to_msg_type()?

Additional parsing should not be needed.

 
> If we can, then output_interpreted_node() can be re-factored so we are
> not parsing the same data twice for every event.

It should be safe to remove the "old code". I don't think 
audit_name_to_msg_type() originally did the fixup. I think it was added when 
libauparse needed the same thing.


> I am uncertain what effect of accepting this additional format would
> have when adding rules to the running audit system - i.e.
> audit_name_to_msg_type() is called by autrace/auditctl when parsing
> rules (ie the msgtype field name).

I think ausearch-report.c might be the place that needs updating.

-Steve

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Burn Alting]

On Wed, 2014-10-01 at 14:54 -0400, Steve Grubb wrote:
> On Monday, September 29, 2014 12:41:23 PM Burn Alting wrote:
> > In lib/lookup_table.c:audit_name_to_msg_type(), the event type value is
> > parsed and converted to an integer as per,
> > 
> > Given
> >         type=<type_value>
> > then
> >         <type_value>
> > is parsed for
> >         - a known string
> >         - a long integer number, n, found in the specific string
> > 		"UNKNOWN[n]"
> >         - a long integer number, n, found in the specific string
> > 		"n"
> > 
> > In src/ausearch-report.c:output_interpreted_node() it additionally
> > parses for a <type_value> of
> >         - a long integer number, n, found in the string "[^\[]*[n].*"
> > i.e.
> >         type=something[n]something_else
> 
> This is specifically a fixup for the UNKNOWN[####] case. There is no other value 
> it can be. This originates here:
> 
> https://fedorahosted.org/audit/browser/trunk/src/auditd-event.c#L1054
> 
> 
> > Is there any reason against adding this additional parsing into
> > lib/lookup_table.c:audit_name_to_msg_type()?
> 
> Additional parsing should not be needed.
> 
>  
> > If we can, then output_interpreted_node() can be re-factored so we are
> > not parsing the same data twice for every event.
> 
> It should be safe to remove the "old code". I don't think 
> audit_name_to_msg_type() originally did the fixup. I think it was added when 
> libauparse needed the same thing.
> 
> 
> > I am uncertain what effect of accepting this additional format would
> > have when adding rules to the running audit system - i.e.
> > audit_name_to_msg_type() is called by autrace/auditctl when parsing
> > rules (ie the msgtype field name).
> 
> I think ausearch-report.c might be the place that needs updating.

So, could we modify output_interpreted_node() to no longer re-parse the 
   [node=<node>] type=<type> msg=audit(<epochsecs>.<msecs>:<serial>)
header and pass both the lnode and llist->e which has this data already
as the code 
          if (num == -1) {
              // see if we are older and wiser now.
              bptr = strchr(str, '[');
              if (bptr && bptr < ptr) {
                  char *eptr;
                  bptr++;
                  eptr = strchr(bptr, ']');
                  if (eptr) {
                       *eptr = 0;
                       errno = 0;
                       num = strtoul(bptr, NULL, 10);
                       *eptr = ']';
                       if (errno)
                           num = -1;
                  }
               }
         }
which parses for
    type=.*[n].*
is no longer needed as we don't have that format any more?


> 
> -Steve

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Steve Grubb]

On Thursday, October 02, 2014 07:08:13 AM Burn Alting wrote:
> On Wed, 2014-10-01 at 14:54 -0400, Steve Grubb wrote:
> > > I am uncertain what effect of accepting this additional format would
> > > have when adding rules to the running audit system - i.e.
> > > audit_name_to_msg_type() is called by autrace/auditctl when parsing
> > > rules (ie the msgtype field name).
> > 
> > I think ausearch-report.c might be the place that needs updating.
> 
> So, could we modify output_interpreted_node() to no longer re-parse the
>    [node=<node>] type=<type> msg=audit(<epochsecs>.<msecs>:<serial>)
> header and pass both the lnode and llist->e which has this data already
> as the code
>           if (num == -1) {
>               // see if we are older and wiser now.
>               bptr = strchr(str, '[');
>               if (bptr && bptr < ptr) {
>                   char *eptr;
>                   bptr++;
>                   eptr = strchr(bptr, ']');
>                   if (eptr) {
>                        *eptr = 0;
>                        errno = 0;
>                        num = strtoul(bptr, NULL, 10);
>                        *eptr = ']';
>                        if (errno)
>                            num = -1;
>                   }
>                }
>          }
> which parses for
>     type=.*[n].*
> is no longer needed as we don't have that format any more?

That is a very loose check for UNKNOWN[####]. If you see a performance 
improvement by refactoring this function, please send a patch. The output 
needs to be identical to the old way.

Thanks,
-Steve

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Steve Grubb]

On Wednesday, October 01, 2014 05:19:39 PM Steve Grubb wrote:
> On Thursday, October 02, 2014 07:08:13 AM Burn Alting wrote:
> > On Wed, 2014-10-01 at 14:54 -0400, Steve Grubb wrote:
> > > > I am uncertain what effect of accepting this additional format would
> > > > have when adding rules to the running audit system - i.e.
> > > > audit_name_to_msg_type() is called by autrace/auditctl when parsing
> > > > rules (ie the msgtype field name).
> > > 
> > > I think ausearch-report.c might be the place that needs updating.
> > 
> > So, could we modify output_interpreted_node() to no longer re-parse the
> > 
> >    [node=<node>] type=<type> msg=audit(<epochsecs>.<msecs>:<serial>)
> > 
> > header and pass both the lnode and llist->e which has this data already
> > as the code
> > 
> >           if (num == -1) {
> >           
> >               // see if we are older and wiser now.
> >               bptr = strchr(str, '[');
> >               if (bptr && bptr < ptr) {
> >                   char *eptr;
> >                   bptr++;
> >                   eptr = strchr(bptr, ']');
> >                   if (eptr) {
> >                        *eptr = 0;
> >                        errno = 0;
> >                        num = strtoul(bptr, NULL, 10);
> >                        *eptr = ']';
> >                        if (errno)
> >                            num = -1;
> >                   }
> >                }
> >          }
> > 
> > which parses for
> > 
> >     type=.*[n].*
> > 
> > is no longer needed as we don't have that format any more?
> 
> That is a very loose check for UNKNOWN[####]. If you see a performance
> improvement by refactoring this function, please send a patch. The output
> needs to be identical to the old way.

Note that this section of code is on the "slow path". The value of interest 
extraction and matching is the performance critical area. Once an event is 
selected for output, the tty scrolling and buffering probably dominate the 
speed. So, I would take a patch to optimize this, but I don't expect it to 
make a big difference.

-Steve

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Burn Alting]

On Wed, 2014-10-01 at 17:19 -0400, Steve Grubb wrote:
> On Thursday, October 02, 2014 07:08:13 AM Burn Alting wrote:
> > On Wed, 2014-10-01 at 14:54 -0400, Steve Grubb wrote:
> > > > I am uncertain what effect of accepting this additional format would
> > > > have when adding rules to the running audit system - i.e.
> > > > audit_name_to_msg_type() is called by autrace/auditctl when parsing
> > > > rules (ie the msgtype field name).
> > > 
> > > I think ausearch-report.c might be the place that needs updating.
> > 
> > So, could we modify output_interpreted_node() to no longer re-parse the
> >    [node=<node>] type=<type> msg=audit(<epochsecs>.<msecs>:<serial>)
> > header and pass both the lnode and llist->e which has this data already
> > as the code
> >           if (num == -1) {
> >               // see if we are older and wiser now.
> >               bptr = strchr(str, '[');
> >               if (bptr && bptr < ptr) {
> >                   char *eptr;
> >                   bptr++;
> >                   eptr = strchr(bptr, ']');
> >                   if (eptr) {
> >                        *eptr = 0;
> >                        errno = 0;
> >                        num = strtoul(bptr, NULL, 10);
> >                        *eptr = ']';
> >                        if (errno)
> >                            num = -1;
> >                   }
> >                }
> >          }
> > which parses for
> >     type=.*[n].*
> > is no longer needed as we don't have that format any more?
> 
> That is a very loose check for UNKNOWN[####]. If you see a performance 
> improvement by refactoring this function, please send a patch. The output 
> needs to be identical to the old way.
> 
> Thanks,
> -Steve

I can provide a patch to refactor this part of the code, but I want to
confirm there is no longer a need to parse for

  type=some_text '[' integer_type ']' some_other_text

given my refactoring will rely upon the parsing already done by
lib/lookup_table.c:audit_name_to_msg_type(). Remember this routine only
parses for
Given
        type=<type_value> 
then
        <type_value>
is parsed for
        - a known string 
        - a long integer number, n, found in the specific string
                "UNKNOWN[n]"
        - a long integer number, n, found in the specific string
                "n"


Rgds

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Burn Alting]

On Wed, 2014-10-01 at 17:24 -0400, Steve Grubb wrote:
> On Wednesday, October 01, 2014 05:19:39 PM Steve Grubb wrote:
> > On Thursday, October 02, 2014 07:08:13 AM Burn Alting wrote:
> > > On Wed, 2014-10-01 at 14:54 -0400, Steve Grubb wrote:
> > > > > I am uncertain what effect of accepting this additional format would
> > > > > have when adding rules to the running audit system - i.e.
> > > > > audit_name_to_msg_type() is called by autrace/auditctl when parsing
> > > > > rules (ie the msgtype field name).
> > > > 
> > > > I think ausearch-report.c might be the place that needs updating.
> > > 
> > > So, could we modify output_interpreted_node() to no longer re-parse the
> > > 
> > >    [node=<node>] type=<type> msg=audit(<epochsecs>.<msecs>:<serial>)
> > > 
> > > header and pass both the lnode and llist->e which has this data already
> > > as the code
> > > 
> > >           if (num == -1) {
> > >           
> > >               // see if we are older and wiser now.
> > >               bptr = strchr(str, '[');
> > >               if (bptr && bptr < ptr) {
> > >                   char *eptr;
> > >                   bptr++;
> > >                   eptr = strchr(bptr, ']');
> > >                   if (eptr) {
> > >                        *eptr = 0;
> > >                        errno = 0;
> > >                        num = strtoul(bptr, NULL, 10);
> > >                        *eptr = ']';
> > >                        if (errno)
> > >                            num = -1;
> > >                   }
> > >                }
> > >          }
> > > 
> > > which parses for
> > > 
> > >     type=.*[n].*
> > > 
> > > is no longer needed as we don't have that format any more?
> > 
> > That is a very loose check for UNKNOWN[####]. If you see a performance
> > improvement by refactoring this function, please send a patch. The output
> > needs to be identical to the old way.
> 
> Note that this section of code is on the "slow path". The value of interest 
> extraction and matching is the performance critical area. Once an event is 
> selected for output, the tty scrolling and buffering probably dominate the 
> speed. So, I would take a patch to optimize this, but I don't expect it to 
> make a big difference.
> 
> -Steve

I'm not really after a performance gain, but rather, positioning
output_interpreted_node() to possibly support multiple formats for
output [see my other posts].


Rgds

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Steve Grubb]

On Thursday, October 02, 2014 07:52:47 AM Burn Alting wrote:
> On Wed, 2014-10-01 at 17:19 -0400, Steve Grubb wrote:
> > On Thursday, October 02, 2014 07:08:13 AM Burn Alting wrote:
> > > On Wed, 2014-10-01 at 14:54 -0400, Steve Grubb wrote:
> > > > > I am uncertain what effect of accepting this additional format would
> > > > > have when adding rules to the running audit system - i.e.
> > > > > audit_name_to_msg_type() is called by autrace/auditctl when parsing
> > > > > rules (ie the msgtype field name).
> > > > 
> > > > I think ausearch-report.c might be the place that needs updating.
> > > 
> > > So, could we modify output_interpreted_node() to no longer re-parse the
> > > 
> > >    [node=<node>] type=<type> msg=audit(<epochsecs>.<msecs>:<serial>)
> > > 
> > > header and pass both the lnode and llist->e which has this data already
> > > as the code
> > > 
> > >           if (num == -1) {
> > >           
> > >               // see if we are older and wiser now.
> > >               bptr = strchr(str, '[');
> > >               if (bptr && bptr < ptr) {
> > >               
> > >                   char *eptr;
> > >                   bptr++;
> > >                   eptr = strchr(bptr, ']');
> > >                   if (eptr) {
> > >                   
> > >                        *eptr = 0;
> > >                        errno = 0;
> > >                        num = strtoul(bptr, NULL, 10);
> > >                        *eptr = ']';
> > >                        if (errno)
> > >                        
> > >                            num = -1;
> > >                   
> > >                   }
> > >                
> > >                }
> > >          
> > >          }
> > > 
> > > which parses for
> > > 
> > >     type=.*[n].*
> > > 
> > > is no longer needed as we don't have that format any more?
> > 
> > That is a very loose check for UNKNOWN[####]. If you see a performance
> > improvement by refactoring this function, please send a patch. The output
> > needs to be identical to the old way.
> > 
> > Thanks,
> > -Steve
> 
> I can provide a patch to refactor this part of the code, but I want to
> confirm there is no longer a need to parse for
> 
>   type=some_text '[' integer_type ']' some_other_text

While this may have been implied by the code, the fact is that [ ] would only 
be in type fields when its unknown[####].


> given my refactoring will rely upon the parsing already done by
> lib/lookup_table.c:audit_name_to_msg_type(). Remember this routine only
> parses for
> Given
>         type=<type_value>
> then
>         <type_value>
> is parsed for
>         - a known string
>         - a long integer number, n, found in the specific string
>                 "UNKNOWN[n]"
>         - a long integer number, n, found in the specific string
>                 "n"

These 3 formats are all that it can ever be. So, I think you have a correct 
understanding.

-Steve

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Burn Alting]

Thanks Steve,

Patch to follow this weekend.

Rgds

On Wed, 2014-10-01 at 18:28 -0400, Steve Grubb wrote:
> On Thursday, October 02, 2014 07:52:47 AM Burn Alting wrote:
> > On Wed, 2014-10-01 at 17:19 -0400, Steve Grubb wrote:
> > > On Thursday, October 02, 2014 07:08:13 AM Burn Alting wrote:
> > > > On Wed, 2014-10-01 at 14:54 -0400, Steve Grubb wrote:
> > > > > > I am uncertain what effect of accepting this additional format would
> > > > > > have when adding rules to the running audit system - i.e.
> > > > > > audit_name_to_msg_type() is called by autrace/auditctl when parsing
> > > > > > rules (ie the msgtype field name).
> > > > > 
> > > > > I think ausearch-report.c might be the place that needs updating.
> > > > 
> > > > So, could we modify output_interpreted_node() to no longer re-parse the
> > > > 
> > > >    [node=<node>] type=<type> msg=audit(<epochsecs>.<msecs>:<serial>)
> > > > 
> > > > header and pass both the lnode and llist->e which has this data already
> > > > as the code
> > > > 
> > > >           if (num == -1) {
> > > >           
> > > >               // see if we are older and wiser now.
> > > >               bptr = strchr(str, '[');
> > > >               if (bptr && bptr < ptr) {
> > > >               
> > > >                   char *eptr;
> > > >                   bptr++;
> > > >                   eptr = strchr(bptr, ']');
> > > >                   if (eptr) {
> > > >                   
> > > >                        *eptr = 0;
> > > >                        errno = 0;
> > > >                        num = strtoul(bptr, NULL, 10);
> > > >                        *eptr = ']';
> > > >                        if (errno)
> > > >                        
> > > >                            num = -1;
> > > >                   
> > > >                   }
> > > >                
> > > >                }
> > > >          
> > > >          }
> > > > 
> > > > which parses for
> > > > 
> > > >     type=.*[n].*
> > > > 
> > > > is no longer needed as we don't have that format any more?
> > > 
> > > That is a very loose check for UNKNOWN[####]. If you see a performance
> > > improvement by refactoring this function, please send a patch. The output
> > > needs to be identical to the old way.
> > > 
> > > Thanks,
> > > -Steve
> > 
> > I can provide a patch to refactor this part of the code, but I want to
> > confirm there is no longer a need to parse for
> > 
> >   type=some_text '[' integer_type ']' some_other_text
> 
> While this may have been implied by the code, the fact is that [ ] would only 
> be in type fields when its unknown[####].
> 
> 
> > given my refactoring will rely upon the parsing already done by
> > lib/lookup_table.c:audit_name_to_msg_type(). Remember this routine only
> > parses for
> > Given
> >         type=<type_value>
> > then
> >         <type_value>
> > is parsed for
> >         - a known string
> >         - a long integer number, n, found in the specific string
> >                 "UNKNOWN[n]"
> >         - a long integer number, n, found in the specific string
> >                 "n"
> 
> These 3 formats are all that it can ever be. So, I think you have a correct 
> understanding.
> 
> -Steve

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Burn Alting]

[-- Attachment #1: Type: text/plain, Size: 3918 bytes --]

All,

This patch prevents ausearch from parsing an event's "header" of node,
type, time and serial twice. That is
[node=<node>] type=<type> msg=audit(<epochsecs>.<msecs>:<serial>)

It does this by passing the llist->e value to output_interpreted_node()
as this structure holds the already parsed event "header" information.

The code does not change the ausearch -i output.

Rgds

On Thu, 2014-10-02 at 19:29 +1000, Burn Alting wrote:
> Thanks Steve,
> 
> Patch to follow this weekend.
> 
> Rgds
> 
> On Wed, 2014-10-01 at 18:28 -0400, Steve Grubb wrote:
> > On Thursday, October 02, 2014 07:52:47 AM Burn Alting wrote:
> > > On Wed, 2014-10-01 at 17:19 -0400, Steve Grubb wrote:
> > > > On Thursday, October 02, 2014 07:08:13 AM Burn Alting wrote:
> > > > > On Wed, 2014-10-01 at 14:54 -0400, Steve Grubb wrote:
> > > > > > > I am uncertain what effect of accepting this additional format would
> > > > > > > have when adding rules to the running audit system - i.e.
> > > > > > > audit_name_to_msg_type() is called by autrace/auditctl when parsing
> > > > > > > rules (ie the msgtype field name).
> > > > > > 
> > > > > > I think ausearch-report.c might be the place that needs updating.
> > > > > 
> > > > > So, could we modify output_interpreted_node() to no longer re-parse the
> > > > > 
> > > > >    [node=<node>] type=<type> msg=audit(<epochsecs>.<msecs>:<serial>)
> > > > > 
> > > > > header and pass both the lnode and llist->e which has this data already
> > > > > as the code
> > > > > 
> > > > >           if (num == -1) {
> > > > >           
> > > > >               // see if we are older and wiser now.
> > > > >               bptr = strchr(str, '[');
> > > > >               if (bptr && bptr < ptr) {
> > > > >               
> > > > >                   char *eptr;
> > > > >                   bptr++;
> > > > >                   eptr = strchr(bptr, ']');
> > > > >                   if (eptr) {
> > > > >                   
> > > > >                        *eptr = 0;
> > > > >                        errno = 0;
> > > > >                        num = strtoul(bptr, NULL, 10);
> > > > >                        *eptr = ']';
> > > > >                        if (errno)
> > > > >                        
> > > > >                            num = -1;
> > > > >                   
> > > > >                   }
> > > > >                
> > > > >                }
> > > > >          
> > > > >          }
> > > > > 
> > > > > which parses for
> > > > > 
> > > > >     type=.*[n].*
> > > > > 
> > > > > is no longer needed as we don't have that format any more?
> > > > 
> > > > That is a very loose check for UNKNOWN[####]. If you see a performance
> > > > improvement by refactoring this function, please send a patch. The output
> > > > needs to be identical to the old way.
> > > > 
> > > > Thanks,
> > > > -Steve
> > > 
> > > I can provide a patch to refactor this part of the code, but I want to
> > > confirm there is no longer a need to parse for
> > > 
> > >   type=some_text '[' integer_type ']' some_other_text
> > 
> > While this may have been implied by the code, the fact is that [ ] would only 
> > be in type fields when its unknown[####].
> > 
> > 
> > > given my refactoring will rely upon the parsing already done by
> > > lib/lookup_table.c:audit_name_to_msg_type(). Remember this routine only
> > > parses for
> > > Given
> > >         type=<type_value>
> > > then
> > >         <type_value>
> > > is parsed for
> > >         - a known string
> > >         - a long integer number, n, found in the specific string
> > >                 "UNKNOWN[n]"
> > >         - a long integer number, n, found in the specific string
> > >                 "n"
> > 
> > These 3 formats are all that it can ever be. So, I think you have a correct 
> > understanding.
> > 
> > -Steve
> 
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit


[-- Attachment #2: audit-2.4_ausearch_refactor_01.patch --]
[-- Type: text/x-patch, Size: 4026 bytes --]

diff -Npru audit-2.4/src/ausearch-report.c audit-2.4_ausearch_refactor_01/src/ausearch-report.c
--- audit-2.4/src/ausearch-report.c	2014-08-25 02:39:20.000000000 +1000
+++ audit-2.4_ausearch_refactor_01/src/ausearch-report.c	2014-10-06 17:32:43.916773469 +1100
@@ -37,7 +37,7 @@
 static void output_raw(llist *l);
 static void output_default(llist *l);
 static void output_interpreted(llist *l);
-static void output_interpreted_node(const lnode *n);
+static void output_interpreted_node(const lnode *n, const event *e);
 static void interpret(char *name, char *val, int comma, int rtype);
 
 /* The machine based on elf type */
@@ -125,10 +125,10 @@ static void output_interpreted(llist *l)
 		return;
 	}
 	if (n->type >= AUDIT_DAEMON_START && n->type < AUDIT_SYSCALL) 
-		output_interpreted_node(n);
+		output_interpreted_node(n, &(l->e));
 	else {
 		do {
-			output_interpreted_node(n);
+			output_interpreted_node(n, &(l->e));
 		} while ((n=list_prev(l)));
 	}
 }
@@ -137,21 +137,25 @@ static void output_interpreted(llist *l)
  * This function will cycle through a single record and lookup each field's
  * value that it finds. 
  */
-static void output_interpreted_node(const lnode *n)
+static void output_interpreted_node(const lnode *n, const event *e)
 {
 	char *ptr, *str = n->message, *node = NULL;
 	int found, comma = 0;
+	int num = n->type;
+	struct tm *btm;
+	char tmp[32];
 
 	// Reset these because each record could be different
 	machine = -1;
 	cur_syscall = -1;
 
-	/* Check and see if we start with a node */
-	if (str[0] == 'n') {
-		ptr=strchr(str, ' ');
-		if (ptr) {
-			*ptr = 0;
-			node = str;
+	/* Check and see if we start with a node
+ 	 * If we do, and there is a space in the line
+ 	 * move the pointer to the first character past
+ 	 * the space
+  	 */
+	if (e->node) {
+		if ((ptr=strchr(str, ' ')) != NULL) {
 			str = ptr+1;
 		}
 	}
@@ -161,76 +165,34 @@ static void output_interpreted_node(cons
 	if (ptr == NULL) {
 		fprintf(stderr, "can't find time stamp\n");
 		return;
-	} else {
-		time_t t;
-		int milli,num = n->type;
-		unsigned long serial;
-		struct tm *btm;
-		char tmp[32];
-		const char *bptr;
-
-		*ptr++ = 0;
-		if (num == -1) {
-			// see if we are older and wiser now.
-			bptr = strchr(str, '[');
-			if (bptr && bptr < ptr) {
-				char *eptr;
-				bptr++;
-				eptr = strchr(bptr, ']');
-				if (eptr) {
-					*eptr = 0;
-					errno = 0;
-					num = strtoul(bptr, NULL, 10);
-					*eptr = ']';
-					if (errno) 
-						num = -1;
-				}
-			}
-		}
+	}
+
+	*ptr++ = 0;	/* move to the start of the timestamp */
 
-		// print everything up to it.
-		if (num >= 0) {
-			bptr = audit_msg_type_to_name(num);
-			if (bptr) {
-				if (node)
-					printf("%s ", node);
-				printf("type=%s msg=audit(", bptr);
-				goto no_print;
-			}
-		} 
-		if (node)
-			printf("%s ", node);
-		printf("%s(", str);
+	// print everything up to it.
+	if (num >= 0) {
+		const char	* bptr;
+		bptr = audit_msg_type_to_name(num);
+		if (bptr) {
+			if (e->node)
+				printf("node=%s ", e->node);
+			printf("type=%s msg=audit(", bptr);
+			goto no_print;
+		}
+	} 
+	if (e->node)
+		printf("node=%s ", e->node);
+	printf("%s(", str);
 no_print:
 
-		// output formatted time.
-		str = strchr(ptr, '.');
-		if (str == NULL)
-			return;
-		*str++ = 0;
-		errno = 0;
-		t = strtoul(ptr, NULL, 10);
-		if (errno)
-			return;
-		ptr = strchr(str, ':');
-		if (ptr == NULL)
-			return;
-		*ptr++ = 0;
-		milli = strtoul(str, NULL, 10);
-		if (errno)
-			return;
-		str = strchr(ptr, ')');
-		if(str == NULL)
-			return;
-		*str++ = 0;
-		serial = strtoul(ptr, NULL, 10);
-		if (errno)
-			return;
-		btm = localtime(&t);
-		strftime(tmp, sizeof(tmp), "%x %T", btm);
-		printf("%s", tmp);
-		printf(".%03d:%lu) ", milli, serial);
-	}
+	str = strchr(ptr, ')');
+	if(str == NULL)
+		return;
+	*str++ = 0;
+	btm = localtime(&e->sec);
+	strftime(tmp, sizeof(tmp), "%x %T", btm);
+	printf("%s", tmp);
+	printf(".%03d:%lu) ", e->milli, e->serial);
 
 	if (n->type == AUDIT_SYSCALL) { 
 		a0 = n->a0;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: Refactoring src/ausearch-report.c:output_interpreted_node()

[Steve Grubb]

On Tue, 07 Oct 2014 20:31:30 +1100
Burn Alting <burn@swtf.dyndns.org> wrote:
> This patch prevents ausearch from parsing an event's "header" of node,
> type, time and serial twice. That is
> [node=<node>] type=<type> msg=audit(<epochsecs>.<msecs>:<serial>)
> 
> It does this by passing the llist->e value to
> output_interpreted_node() as this structure holds the already parsed
> event "header" information.
> 
> The code does not change the ausearch -i output.

Thanks for the patch. I've applied it locally and it seems to work
fine. Will commit it as soon as I can.

-Steve