1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v2] util: audit: Fix logging an error when kernel lacks audit support

Erik Skultety posted 1 patch 5 years, 3 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/aa99f6c7600ca5fa098c1b97b0cf1ffaa3227852.1547650666.git.eskultet@redhat.com
src/remote/remote_daemon.c |  2 +-
src/util/viraudit.c        | 16 ++--------------
src/util/viraudit.h        |  2 +-
3 files changed, 4 insertions(+), 16 deletions(-)
[libvirt] [PATCH v2] util: audit: Fix logging an error when kernel lacks audit support
Posted by Erik Skultety 5 years, 3 months ago 
Based on an upstream discussion, reporting the errno is useful for the
user to know why audit isn't supported. Even though having an error in
the logs might look concerning when 'audit_log=1', it also denotes that
audit is only going to be used if it's available, continuing normally
if it's unavailable for whatever reason.

Partially reverts commit 4199c2f221c.

https://bugzilla.redhat.com/show_bug.cgi?id=1596119

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/remote/remote_daemon.c |  2 +-
 src/util/viraudit.c        | 16 ++--------------
 src/util/viraudit.h        |  2 +-
 3 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 3be3ad02fc..ededef97b4 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -1380,7 +1380,7 @@ int main(int argc, char **argv) {

     if (config->audit_level) {
         VIR_DEBUG("Attempting to configure auditing subsystem");
-        if (virAuditOpen(config->audit_level) < 0) {
+        if (virAuditOpen() < 0) {
             if (config->audit_level > 1) {
                 ret = VIR_DAEMON_ERR_AUDIT;
                 goto cleanup;
diff --git a/src/util/viraudit.c b/src/util/viraudit.c
index a02e5b36fd..135d0e626a 100644
--- a/src/util/viraudit.c
+++ b/src/util/viraudit.c
@@ -54,23 +54,11 @@ static int auditfd = -1;
 #endif
 static bool auditlog;

-int virAuditOpen(unsigned int audit_level ATTRIBUTE_UNUSED)
+int virAuditOpen(void)
 {
 #if WITH_AUDIT
     if ((auditfd = audit_open()) < 0) {
-        /* You get these error codes only when the kernel does not
-         * have audit compiled in or it's disabled (e.g. by the kernel
-         * cmdline) */
-        if (errno == EINVAL || errno == EPROTONOSUPPORT ||
-            errno == EAFNOSUPPORT) {
-            if (audit_level < 2)
-                VIR_INFO("Audit is not supported by the kernel");
-            else
-                virReportError(VIR_FROM_THIS, "%s", _("Audit is not supported by the kernel"));
-        } else {
-            virReportSystemError(errno, "%s", _("Unable to initialize audit layer"));
-        }
-
+        virReportSystemError(errno, "%s", _("Unable to initialize audit layer"));
         return -1;
     }

diff --git a/src/util/viraudit.h b/src/util/viraudit.h
index 66605b16b5..7fbc28ba9b 100644
--- a/src/util/viraudit.h
+++ b/src/util/viraudit.h
@@ -31,7 +31,7 @@ typedef enum {
     VIR_AUDIT_RECORD_RESOURCE,
 } virAuditRecordType;

-int virAuditOpen(unsigned int audit_level);
+int virAuditOpen(void);

 void virAuditLog(bool enabled);

--
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] util: audit: Fix logging an error when kernel lacks audit support
Posted by Daniel P. Berrangé 5 years, 3 months ago 
On Wed, Jan 16, 2019 at 03:58:24PM +0100, Erik Skultety wrote:
> Based on an upstream discussion, reporting the errno is useful for the
> user to know why audit isn't supported. Even though having an error in
> the logs might look concerning when 'audit_log=1', it also denotes that
> audit is only going to be used if it's available, continuing normally
> if it's unavailable for whatever reason.
> 
> Partially reverts commit 4199c2f221c.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1596119
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/remote/remote_daemon.c |  2 +-
>  src/util/viraudit.c        | 16 ++--------------
>  src/util/viraudit.h        |  2 +-
>  3 files changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index 3be3ad02fc..ededef97b4 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -1380,7 +1380,7 @@ int main(int argc, char **argv) {
> 
>      if (config->audit_level) {
>          VIR_DEBUG("Attempting to configure auditing subsystem");
> -        if (virAuditOpen(config->audit_level) < 0) {
> +        if (virAuditOpen() < 0) {
>              if (config->audit_level > 1) {
>                  ret = VIR_DAEMON_ERR_AUDIT;
>                  goto cleanup;
> diff --git a/src/util/viraudit.c b/src/util/viraudit.c
> index a02e5b36fd..135d0e626a 100644
> --- a/src/util/viraudit.c
> +++ b/src/util/viraudit.c
> @@ -54,23 +54,11 @@ static int auditfd = -1;
>  #endif
>  static bool auditlog;
> 
> -int virAuditOpen(unsigned int audit_level ATTRIBUTE_UNUSED)
> +int virAuditOpen(void)
>  {
>  #if WITH_AUDIT
>      if ((auditfd = audit_open()) < 0) {
> -        /* You get these error codes only when the kernel does not
> -         * have audit compiled in or it's disabled (e.g. by the kernel
> -         * cmdline) */
> -        if (errno == EINVAL || errno == EPROTONOSUPPORT ||
> -            errno == EAFNOSUPPORT) {
> -            if (audit_level < 2)
> -                VIR_INFO("Audit is not supported by the kernel");
> -            else
> -                virReportError(VIR_FROM_THIS, "%s", _("Audit is not supported by the kernel"));
> -        } else {
> -            virReportSystemError(errno, "%s", _("Unable to initialize audit layer"));
> -        }
> -
> +        virReportSystemError(errno, "%s", _("Unable to initialize audit layer"));
>          return -1;
>      }

I'm not a fan of this - I don't think we should report stuff in the
logs at level error: if we're treating it as non-fatal.

I think it can be refactored to push all the handling of
audit_level down into virAuditOpen though, instead of having
it split between virAuditOpen & the caller.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] util: audit: Fix logging an error when kernel lacks audit support
Posted by Erik Skultety 5 years, 3 months ago 
On Wed, Jan 16, 2019 at 03:45:06PM +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 16, 2019 at 03:58:24PM +0100, Erik Skultety wrote:
> > Based on an upstream discussion, reporting the errno is useful for the
> > user to know why audit isn't supported. Even though having an error in
> > the logs might look concerning when 'audit_log=1', it also denotes that
> > audit is only going to be used if it's available, continuing normally
> > if it's unavailable for whatever reason.
> >
> > Partially reverts commit 4199c2f221c.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1596119
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> >  src/remote/remote_daemon.c |  2 +-
> >  src/util/viraudit.c        | 16 ++--------------
> >  src/util/viraudit.h        |  2 +-
> >  3 files changed, 4 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> > index 3be3ad02fc..ededef97b4 100644
> > --- a/src/remote/remote_daemon.c
> > +++ b/src/remote/remote_daemon.c
> > @@ -1380,7 +1380,7 @@ int main(int argc, char **argv) {
> >
> >      if (config->audit_level) {
> >          VIR_DEBUG("Attempting to configure auditing subsystem");
> > -        if (virAuditOpen(config->audit_level) < 0) {
> > +        if (virAuditOpen() < 0) {
> >              if (config->audit_level > 1) {
> >                  ret = VIR_DAEMON_ERR_AUDIT;
> >                  goto cleanup;
> > diff --git a/src/util/viraudit.c b/src/util/viraudit.c
> > index a02e5b36fd..135d0e626a 100644
> > --- a/src/util/viraudit.c
> > +++ b/src/util/viraudit.c
> > @@ -54,23 +54,11 @@ static int auditfd = -1;
> >  #endif
> >  static bool auditlog;
> >
> > -int virAuditOpen(unsigned int audit_level ATTRIBUTE_UNUSED)
> > +int virAuditOpen(void)
> >  {
> >  #if WITH_AUDIT
> >      if ((auditfd = audit_open()) < 0) {
> > -        /* You get these error codes only when the kernel does not
> > -         * have audit compiled in or it's disabled (e.g. by the kernel
> > -         * cmdline) */
> > -        if (errno == EINVAL || errno == EPROTONOSUPPORT ||
> > -            errno == EAFNOSUPPORT) {
> > -            if (audit_level < 2)
> > -                VIR_INFO("Audit is not supported by the kernel");
> > -            else
> > -                virReportError(VIR_FROM_THIS, "%s", _("Audit is not supported by the kernel"));
> > -        } else {
> > -            virReportSystemError(errno, "%s", _("Unable to initialize audit layer"));
> > -        }
> > -
> > +        virReportSystemError(errno, "%s", _("Unable to initialize audit layer"));
> >          return -1;
> >      }
>
> I'm not a fan of this - I don't think we should report stuff in the
> logs at level error: if we're treating it as non-fatal.
>
> I think it can be refactored to push all the handling of
> audit_level down into virAuditOpen though, instead of having
> it split between virAuditOpen & the caller.

Okay, honestly, this was my quick way away from this, since I wanted to
populate the log with the errno translation either way and get rid of that
bugzilla, which has literally no priority btw and which looked like a 5min fix,
so I'm not going to put any more effort into this for the time being.

Thanks,
Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.