Re: [PATCH] log: fix false oom error message

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/09/16 12:56, Nikolay Shirokovskiy wrote:
> In case filter is new one an oom error message is reported.
> This bug is introduced in 51b2606f.

oops

 However this error does
> not get into the log in the default configuration. To trigger
> it one need to specify log outputs in LIBVIRT_LOG_OUTPUTS
> variable.

Well, in my understanding, this is only true when the daemon is starting
and the error isn't reported because the outputs are initialized once
the filters' init routine is finished. So if you're preparing a test for
example where you include our internal APIs, then at least the error
would be reported. That's the reason why I'm saying that the statement
about not reporting the error is only true for a daemon restart. Anyhow,
it is a bug that should be fixed but I'm still wondering if we should do
something similar to virLogDefineOutput, which btw also lacks reporting
virReportOOMError() when an allocation fails, since my refactor series
[1] is going to remove these bits of code completely.

[1] https://www.redhat.com/archives/libvir-list/2016-August/msg00907.html

> ---
>  src/util/virlog.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 06f9a60..f5b88b6 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -335,11 +335,12 @@ virLogDefineFilter(const char *match,
>          goto cleanup;
>  
>      virLogFiltersSerial++;
> +    ret = virLogNbFilters - 1;
>   cleanup:
>      virLogUnlock();
>      if (ret < 0)
>          virReportOOMError();
> -    return virLogNbFilters;
> +    return ret;
>  }
>  
>  /**
> 

ACK to the patch though, I slightly adjusted the CM and pushed.
Thanks,
Erik

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]