[libvirt] [PATCH] audit: Log only an info message if audit_level < 2 and audit is not supported

[Michal Privoznik]

On 12/13/2017 10:45 AM, Marc Hartmayer wrote:
> On Wed, Dec 13, 2017 at 10:22 AM +0100, Michal Privoznik <mprivozn at redhat.com> wrote:
>> On 11/27/2017 07:02 PM, Marc Hartmayer wrote:
>>> Replace the error message during startup of libvirtd with an info
>>> message if audit_level < 2 and audit is not supported by the
>>> kernel. Audit is not supported by the current kernel if the kernel
>>> does not have audit compiled in or if audit is disabled (e.g. by the
>>> kernel cmdline).
>>>
>>> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>>> ---
>>>  daemon/libvirtd.c   |  2 +-
>>>  src/util/viraudit.c | 17 +++++++++++++++--
>>>  src/util/viraudit.h |  2 +-
>>>  3 files changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
>>> index 589b32192e3d..6bbff0d45684 100644
>>> --- a/daemon/libvirtd.c
>>> +++ b/daemon/libvirtd.c
>>> @@ -1418,7 +1418,7 @@ int main(int argc, char **argv) {
>>>
>>>      if (config->audit_level) {
>>>          VIR_DEBUG("Attempting to configure auditing subsystem");
>>> -        if (virAuditOpen() < 0) {
>>> +        if (virAuditOpen(config->audit_level) < 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 17e58b3a9574..9b755e384f24 100644
>>> --- a/src/util/viraudit.c
>>> +++ b/src/util/viraudit.c
>>> @@ -55,11 +55,24 @@ static int auditfd = -1;
>>>  #endif
>>>  static bool auditlog;
>>>
>>> -int virAuditOpen(void)
>>> +int virAuditOpen(unsigned int audit_level)
>>
>> @audit_level might be unused if building without AUDIT enabled.
> 
> Hmm, right.
> 
>>
>>>  {
>>>  #if WITH_AUDIT
>>>      if ((auditfd = audit_open()) < 0) {
>>> -        virReportSystemError(errno, "%s", _("Unable to initialize audit layer"));
>>> +        /* 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) {
>>> +            const char msg[] = "Audit is not supported by the kernel";
>>> +            if (audit_level < 2)
>>> +                VIR_INFO("%s", _(msg));
>>
>> This is going to be terrible for translators. If anything, this needs to be:
>>
>> const char *msg = _("error message");
>> if ()
>>   VIR_INFO("%s", msg);
>> else
>>   virReportError(msg);
>>
>> However, I don't think that we need VIR_INFO to have translated messages
>> at all, therefore we can go with just:
>>
>> if ()
>>   VIR_INFO("Audit is not supported");
>> else
>>   virReportError(_("Audit is not supported"));
>
> I think this is fine - but I’m not sure we should omit „by the kernel“.

Oh, it's just me being lazy to write the full error message. I wasn't
focusing on exact phrasing rather than the structure of code. Feel free
to use whatever message you want. The one you already have is fine.

> 
>>> +            else
>>> +                virReportError(VIR_FROM_THIS, "%s", _(msg));
>>> +        } else {
>>> +            virReportSystemError(errno, "%s", _("Unable to initialize audit layer"));
>>> +        }
>>> +
>>
>> Otherwise looking good. In fact, we document the behaviour you're
>> implementing. Wonder how we ended up there.
> 
> Thanks for the review. Shall I send a v2?

Yes please.

Michal