[PATCH v2] audit: add feature audit_lost reset

[PATCH v2] audit: add feature audit_lost reset

[Richard Guy Briggs]

Add a method to reset the audit_lost value.

An AUDIT_SET message with the AUDIT_STATUS_LOST flag set by itself
will return a positive value repesenting the current audit_lost value
and reset the counter to zero.  If AUDIT_STATUS_LOST is not the
only flag set, the reset command will be ignored.  The value sent with
the command is ignored.

An AUDIT_LOST_RESET message will be sent to the listening audit daemon.
The data field will contain a u32 with the positive value of the
audit_lost value when it was reset.

See: https://github.com/linux-audit/audit-kernel/issues/3

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/uapi/linux/audit.h |    2 ++
 kernel/audit.c             |    8 +++++++-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 208df7b..6d38bff 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -70,6 +70,7 @@
 #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
 #define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
 #define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
+#define AUDIT_LOST_RESET	1020	/* Reset the audit_lost value */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */
@@ -325,6 +326,7 @@ enum {
 #define AUDIT_STATUS_RATE_LIMIT		0x0008
 #define AUDIT_STATUS_BACKLOG_LIMIT	0x0010
 #define AUDIT_STATUS_BACKLOG_WAIT_TIME	0x0020
+#define AUDIT_STATUS_LOST		0x0040
 
 #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT	0x00000001
 #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME	0x00000002
diff --git a/kernel/audit.c b/kernel/audit.c
index f1ca116..19cfee0 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -122,7 +122,7 @@
    3) suppressed due to audit_rate_limit
    4) suppressed due to audit_backlog_limit
 */
-static atomic_t    audit_lost = ATOMIC_INIT(0);
+static atomic_t	audit_lost = ATOMIC_INIT(0);
 
 /* The netlink socket. */
 static struct sock *audit_sock;
@@ -920,6 +920,12 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 			if (err < 0)
 				return err;
 		}
+		if (s.mask == AUDIT_STATUS_LOST) {
+			u32 lost = atomic_xchg(&audit_lost, 0);
+
+			audit_send_reply(skb, seq, AUDIT_LOST_RESET, 0, 0, &lost, sizeof(lost));
+			return lost;
+		}
 		break;
 	}
 	case AUDIT_GET_FEATURE:
-- 
1.7.1

Re: [PATCH v2] audit: add feature audit_lost reset

[Paul Moore]

On Sat, Dec 10, 2016 at 6:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Add a method to reset the audit_lost value.
>
> An AUDIT_SET message with the AUDIT_STATUS_LOST flag set by itself
> will return a positive value repesenting the current audit_lost value
> and reset the counter to zero.  If AUDIT_STATUS_LOST is not the
> only flag set, the reset command will be ignored.  The value sent with
> the command is ignored.
>
> An AUDIT_LOST_RESET message will be sent to the listening audit daemon.
> The data field will contain a u32 with the positive value of the
> audit_lost value when it was reset.
>
> See: https://github.com/linux-audit/audit-kernel/issues/3
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/uapi/linux/audit.h |    2 ++
>  kernel/audit.c             |    8 +++++++-
>  2 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 208df7b..6d38bff 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -70,6 +70,7 @@
>  #define AUDIT_TTY_SET          1017    /* Set TTY auditing status */
>  #define AUDIT_SET_FEATURE      1018    /* Turn an audit feature on or off */
>  #define AUDIT_GET_FEATURE      1019    /* Get which features are enabled */
> +#define AUDIT_LOST_RESET       1020    /* Reset the audit_lost value */
>
>  #define AUDIT_FIRST_USER_MSG   1100    /* Userspace messages mostly uninteresting to kernel */
>  #define AUDIT_USER_AVC         1107    /* We filter this differently */
> @@ -325,6 +326,7 @@ enum {
>  #define AUDIT_STATUS_RATE_LIMIT                0x0008
>  #define AUDIT_STATUS_BACKLOG_LIMIT     0x0010
>  #define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
> +#define AUDIT_STATUS_LOST              0x0040
>
>  #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     0x00000001
>  #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f1ca116..19cfee0 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -122,7 +122,7 @@
>     3) suppressed due to audit_rate_limit
>     4) suppressed due to audit_backlog_limit
>  */
> -static atomic_t    audit_lost = ATOMIC_INIT(0);
> +static atomic_t        audit_lost = ATOMIC_INIT(0);
>
>  /* The netlink socket. */
>  static struct sock *audit_sock;
> @@ -920,6 +920,12 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>                         if (err < 0)
>                                 return err;
>                 }
> +               if (s.mask == AUDIT_STATUS_LOST) {
> +                       u32 lost = atomic_xchg(&audit_lost, 0);
> +
> +                       audit_send_reply(skb, seq, AUDIT_LOST_RESET, 0, 0, &lost, sizeof(lost));

I'm not sure it makes much sense to both return the lost value as a
netlink return code as well as send a separate netlink message back to
the controlling task with the same information.  What I meant earlier
was that we would emit an audit record, similar to
audit_log_config_change(), so that the audit log would not only have
information that the status count was reset, but also the subject
information necessary to associate the action with an individual.

Does that make sense?

> +                       return lost;
> +               }
>                 break;
>         }
>         case AUDIT_GET_FEATURE:
> --
> 1.7.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2] audit: add feature audit_lost reset

[Steve Grubb]

On Thursday, December 15, 2016 3:39:16 PM EST Paul Moore wrote:
> On Sat, Dec 10, 2016 at 6:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Add a method to reset the audit_lost value.
> > 
> > An AUDIT_SET message with the AUDIT_STATUS_LOST flag set by itself
> > will return a positive value repesenting the current audit_lost value
> > and reset the counter to zero.  If AUDIT_STATUS_LOST is not the
> > only flag set, the reset command will be ignored.  The value sent with
> > the command is ignored.
> > 
> > An AUDIT_LOST_RESET message will be sent to the listening audit daemon.
> > The data field will contain a u32 with the positive value of the
> > audit_lost value when it was reset.
> > 
> > See: https://github.com/linux-audit/audit-kernel/issues/3
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > 
> >  include/uapi/linux/audit.h |    2 ++
> >  kernel/audit.c             |    8 +++++++-
> >  2 files changed, 9 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 208df7b..6d38bff 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -70,6 +70,7 @@
> > 
> >  #define AUDIT_TTY_SET          1017    /* Set TTY auditing status */
> >  #define AUDIT_SET_FEATURE      1018    /* Turn an audit feature on or off
> >  */ #define AUDIT_GET_FEATURE      1019    /* Get which features are
> >  enabled */> 
> > +#define AUDIT_LOST_RESET       1020    /* Reset the audit_lost value */
> > 
> >  #define AUDIT_FIRST_USER_MSG   1100    /* Userspace messages mostly
> >  uninteresting to kernel */ #define AUDIT_USER_AVC         1107    /* We
> >  filter this differently */> 
> > @@ -325,6 +326,7 @@ enum {
> > 
> >  #define AUDIT_STATUS_RATE_LIMIT                0x0008
> >  #define AUDIT_STATUS_BACKLOG_LIMIT     0x0010
> >  #define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
> > 
> > +#define AUDIT_STATUS_LOST              0x0040
> > 
> >  #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     0x00000001
> >  #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
> > 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f1ca116..19cfee0 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -122,7 +122,7 @@
> > 
> >     3) suppressed due to audit_rate_limit
> >     4) suppressed due to audit_backlog_limit
> >  
> >  */
> > 
> > -static atomic_t    audit_lost = ATOMIC_INIT(0);
> > +static atomic_t        audit_lost = ATOMIC_INIT(0);
> > 
> >  /* The netlink socket. */
> >  static struct sock *audit_sock;
> > 
> > @@ -920,6 +920,12 @@ static int audit_receive_msg(struct sk_buff *skb,
> > struct nlmsghdr *nlh)> 
> >                         if (err < 0)
> >                         
> >                                 return err;
> >                 
> >                 }
> > 
> > +               if (s.mask == AUDIT_STATUS_LOST) {
> > +                       u32 lost = atomic_xchg(&audit_lost, 0);
> > +
> > +                       audit_send_reply(skb, seq, AUDIT_LOST_RESET, 0, 0,
> > &lost, sizeof(lost));
> I'm not sure it makes much sense to both return the lost value as a
> netlink return code as well as send a separate netlink message back to
> the controlling task with the same information.  What I meant earlier
> was that we would emit an audit record, similar to
> audit_log_config_change(), so that the audit log would not only have
> information that the status count was reset, but also the subject
> information necessary to associate the action with an individual.
> 
> Does that make sense?

I'm planning to replace all the config change logging with the 
audit_log_task_simple function I sent so that we have everything. Can we go 
ahead and pull that in so that we can start using it?

Thanks,
-Steve

> > +                       return lost;
> > +               }
> > 
> >                 break;
> >         
> >         }
> > 
> >         case AUDIT_GET_FEATURE:
> > --
> > 1.7.1
> > 
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH v2] audit: add feature audit_lost reset

[Paul Moore]

On Thu, Dec 15, 2016 at 7:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> I'm planning to replace all the config change logging with the
> audit_log_task_simple function I sent so that we have everything. Can we go
> ahead and pull that in so that we can start using it?

There needs to be more than one user of the function to make it
worthwhile; so far that function has only been proposed with a single
user.  Propose it with multiple users and we can look at it seriously.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2] audit: add feature audit_lost reset

[Steve Grubb]

On Thursday, December 15, 2016 7:50:48 PM EST Paul Moore wrote:
> On Thu, Dec 15, 2016 at 7:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > I'm planning to replace all the config change logging with the
> > audit_log_task_simple function I sent so that we have everything. Can we
> > go ahead and pull that in so that we can start using it?
> 
> There needs to be more than one user of the function to make it
> worthwhile; so far that function has only been proposed with a single
> user.  Propose it with multiple users and we can look at it seriously.

That's because I have several unrelated patches that use it. Do you want me to 
send all of them at once? There's going to be at least 5 users of the 
function. Possibly more. I want it to be the default for all future events 
added because it concisely gives the necessary information for well-formed 
events.

-Steve

Re: [PATCH v2] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-15 22:12, Steve Grubb wrote:
> On Thursday, December 15, 2016 7:50:48 PM EST Paul Moore wrote:
> > On Thu, Dec 15, 2016 at 7:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > > I'm planning to replace all the config change logging with the
> > > audit_log_task_simple function I sent so that we have everything. Can we
> > > go ahead and pull that in so that we can start using it?
> > 
> > There needs to be more than one user of the function to make it
> > worthwhile; so far that function has only been proposed with a single
> > user.  Propose it with multiple users and we can look at it seriously.
> 
> That's because I have several unrelated patches that use it. Do you want me to 
> send all of them at once? There's going to be at least 5 users of the 
> function. Possibly more. I want it to be the default for all future events 
> added because it concisely gives the necessary information for well-formed 
> events.

I'd send the audit_log_task_simple() patch alone, then send each feature
that uses it in a separate patch set.  Failing that, send it as a
separate patch in the first patch set to make it available for all, then
follow it with more separate patchsets for other events.

There is a chicken and egg problem here.

> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH v2] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-15 15:39, Paul Moore wrote:
> On Sat, Dec 10, 2016 at 6:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Add a method to reset the audit_lost value.
> >
> > An AUDIT_SET message with the AUDIT_STATUS_LOST flag set by itself
> > will return a positive value repesenting the current audit_lost value
> > and reset the counter to zero.  If AUDIT_STATUS_LOST is not the
> > only flag set, the reset command will be ignored.  The value sent with
> > the command is ignored.
> >
> > An AUDIT_LOST_RESET message will be sent to the listening audit daemon.
> > The data field will contain a u32 with the positive value of the
> > audit_lost value when it was reset.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/3
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/uapi/linux/audit.h |    2 ++
> >  kernel/audit.c             |    8 +++++++-
> >  2 files changed, 9 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 208df7b..6d38bff 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -70,6 +70,7 @@
> >  #define AUDIT_TTY_SET          1017    /* Set TTY auditing status */
> >  #define AUDIT_SET_FEATURE      1018    /* Turn an audit feature on or off */
> >  #define AUDIT_GET_FEATURE      1019    /* Get which features are enabled */
> > +#define AUDIT_LOST_RESET       1020    /* Reset the audit_lost value */
> >
> >  #define AUDIT_FIRST_USER_MSG   1100    /* Userspace messages mostly uninteresting to kernel */
> >  #define AUDIT_USER_AVC         1107    /* We filter this differently */
> > @@ -325,6 +326,7 @@ enum {
> >  #define AUDIT_STATUS_RATE_LIMIT                0x0008
> >  #define AUDIT_STATUS_BACKLOG_LIMIT     0x0010
> >  #define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
> > +#define AUDIT_STATUS_LOST              0x0040
> >
> >  #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     0x00000001
> >  #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f1ca116..19cfee0 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -122,7 +122,7 @@
> >     3) suppressed due to audit_rate_limit
> >     4) suppressed due to audit_backlog_limit
> >  */
> > -static atomic_t    audit_lost = ATOMIC_INIT(0);
> > +static atomic_t        audit_lost = ATOMIC_INIT(0);
> >
> >  /* The netlink socket. */
> >  static struct sock *audit_sock;
> > @@ -920,6 +920,12 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >                         if (err < 0)
> >                                 return err;
> >                 }
> > +               if (s.mask == AUDIT_STATUS_LOST) {
> > +                       u32 lost = atomic_xchg(&audit_lost, 0);
> > +
> > +                       audit_send_reply(skb, seq, AUDIT_LOST_RESET, 0, 0, &lost, sizeof(lost));
> 
> I'm not sure it makes much sense to both return the lost value as a
> netlink return code as well as send a separate netlink message back to
> the controlling task with the same information.  What I meant earlier
> was that we would emit an audit record, similar to
> audit_log_config_change(), so that the audit log would not only have
> information that the status count was reset, but also the subject
> information necessary to associate the action with an individual.

I would argue that both are useful, but I missed your point about having
the record sent to the regular queue rather than sent to the process
that is doing the reset.  The process doing the reset will see that
message, but auditd won't, so it won't end up in the log itself, which
was the whole point of sending the notice of an action that requires
trackng.

I'll respin.  I still see value in returing a +ve value to the caller to
detect that feature.

> Does that make sense?

It does.  (The discussion of audit_log_task_simple was a hijack of this
thread that really belongs in a new thread or at least a new subject
name).

> > +                       return lost;
> > +               }
> >                 break;
> >         }
> >         case AUDIT_GET_FEATURE:
> 
> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH v2] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-15 19:22, Steve Grubb wrote:
> On Thursday, December 15, 2016 3:39:16 PM EST Paul Moore wrote:
> > On Sat, Dec 10, 2016 at 6:52 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Add a method to reset the audit_lost value.
> > > 
> > > An AUDIT_SET message with the AUDIT_STATUS_LOST flag set by itself
> > > will return a positive value repesenting the current audit_lost value
> > > and reset the counter to zero.  If AUDIT_STATUS_LOST is not the
> > > only flag set, the reset command will be ignored.  The value sent with
> > > the command is ignored.
> > > 
> > > An AUDIT_LOST_RESET message will be sent to the listening audit daemon.
> > > The data field will contain a u32 with the positive value of the
> > > audit_lost value when it was reset.
> > > 
> > > See: https://github.com/linux-audit/audit-kernel/issues/3
> > > 
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > 
> > >  include/uapi/linux/audit.h |    2 ++
> > >  kernel/audit.c             |    8 +++++++-
> > >  2 files changed, 9 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index 208df7b..6d38bff 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -70,6 +70,7 @@
> > > 
> > >  #define AUDIT_TTY_SET          1017    /* Set TTY auditing status */
> > >  #define AUDIT_SET_FEATURE      1018    /* Turn an audit feature on or off
> > >  */ #define AUDIT_GET_FEATURE      1019    /* Get which features are
> > >  enabled */> 
> > > +#define AUDIT_LOST_RESET       1020    /* Reset the audit_lost value */
> > > 
> > >  #define AUDIT_FIRST_USER_MSG   1100    /* Userspace messages mostly
> > >  uninteresting to kernel */ #define AUDIT_USER_AVC         1107    /* We
> > >  filter this differently */> 
> > > @@ -325,6 +326,7 @@ enum {
> > > 
> > >  #define AUDIT_STATUS_RATE_LIMIT                0x0008
> > >  #define AUDIT_STATUS_BACKLOG_LIMIT     0x0010
> > >  #define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
> > > 
> > > +#define AUDIT_STATUS_LOST              0x0040
> > > 
> > >  #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     0x00000001
> > >  #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
> > > 
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index f1ca116..19cfee0 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -122,7 +122,7 @@
> > > 
> > >     3) suppressed due to audit_rate_limit
> > >     4) suppressed due to audit_backlog_limit
> > >  
> > >  */
> > > 
> > > -static atomic_t    audit_lost = ATOMIC_INIT(0);
> > > +static atomic_t        audit_lost = ATOMIC_INIT(0);
> > > 
> > >  /* The netlink socket. */
> > >  static struct sock *audit_sock;
> > > 
> > > @@ -920,6 +920,12 @@ static int audit_receive_msg(struct sk_buff *skb,
> > > struct nlmsghdr *nlh)> 
> > >                         if (err < 0)
> > >                         
> > >                                 return err;
> > >                 
> > >                 }
> > > 
> > > +               if (s.mask == AUDIT_STATUS_LOST) {
> > > +                       u32 lost = atomic_xchg(&audit_lost, 0);
> > > +
> > > +                       audit_send_reply(skb, seq, AUDIT_LOST_RESET, 0, 0,
> > > &lost, sizeof(lost));
> > I'm not sure it makes much sense to both return the lost value as a
> > netlink return code as well as send a separate netlink message back to
> > the controlling task with the same information.  What I meant earlier
> > was that we would emit an audit record, similar to
> > audit_log_config_change(), so that the audit log would not only have
> > information that the status count was reset, but also the subject
> > information necessary to associate the action with an individual.
> > 
> > Does that make sense?
> 
> I'm planning to replace all the config change logging with the 
> audit_log_task_simple function I sent so that we have everything. Can we go 
> ahead and pull that in so that we can start using it?

I was too late for this kernel release even with the first version of
this patch, so now we have plenty of time to get this right and have it
sit in next for a while.

> Thanks,
> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH v2] audit: add feature audit_lost reset

[Paul Moore]

On Thu, Dec 15, 2016 at 10:39 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-12-15 22:12, Steve Grubb wrote:
>> On Thursday, December 15, 2016 7:50:48 PM EST Paul Moore wrote:
>> > On Thu, Dec 15, 2016 at 7:22 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > > I'm planning to replace all the config change logging with the
>> > > audit_log_task_simple function I sent so that we have everything. Can we
>> > > go ahead and pull that in so that we can start using it?
>> >
>> > There needs to be more than one user of the function to make it
>> > worthwhile; so far that function has only been proposed with a single
>> > user.  Propose it with multiple users and we can look at it seriously.
>>
>> That's because I have several unrelated patches that use it. Do you want me to
>> send all of them at once? There's going to be at least 5 users of the
>> function. Possibly more. I want it to be the default for all future events
>> added because it concisely gives the necessary information for well-formed
>> events.
>
> I'd send the audit_log_task_simple() patch alone, then send each feature
> that uses it in a separate patch set.  Failing that, send it as a
> separate patch in the first patch set to make it available for all, then
> follow it with more separate patchsets for other events.
>
> There is a chicken and egg problem here.

The extremely safe way to do this would be to submit the unrelated
patches first, each written to *not* make use of your new
consolidation function, then submit a single patch which adds the
function and integrates it with the others.

This approach also has the advantage that you are able to submit fixes
as you run across then and not have to wait for everything.  I know
you already have at least one patch ready to, you just need to remove
the references to audit_log_task_simple.

-- 
paul moore
www.paul-moore.com