On 12/13/2017 10:45 AM, Marc Hartmayer wrote:
> On Wed, Dec 13, 2017 at 10:22 AM +0100, Michal Privoznik <mprivozn@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
