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“.
>> + 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?
>
> Michal
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
