Hi Michal,
Thank you for your comments and improvements on the patch.
> diff --git i/src/logging/log_cleaner.c w/src/logging/log_cleaner.c
> index 4ee91843aa..9f987b02b5 100644
> --- i/src/logging/log_cleaner.c
> +++ w/src/logging/log_cleaner.c
> @@ -82,10 +82,8 @@ virLogCleanerParseFilename(const char *path,
> *rotated_index = 0;
> rotated_index_str = g_match_info_fetch(matchInfo, 3);
>
> - if (!rotated_index_str)
> - return chain_prefix;
> -
> - if (virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
> + if (rotated_index_str && STRNEQ(rotated_index_str, "") &&
> + virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to parse rotated index from '%1$s'"),
> rotated_index_str);
^ I tested the above patch and it worked.
> Can you please send v2?
Do you want to submit the patch and keep the author since you
basically wrote the patch, or you want me to send a patch with you as
co-author?
Happy to do it either way.
Kind regards,
David Negreira.
On Tue, May 21, 2024 at 1:05 PM Michal Prívozník <mprivozn@xxxxxxxxxx> wrote:
>
> On 5/21/24 11:11, David Negreira wrote:
> > Hi,
> >
> > For the reproducer I have enabled the garbage collection and debugging
> > on virtlogd.conf file:
> > - grep -v ^# /etc/libvirt/virtlogd.conf
> > log_filters="1:logging 4:object 4:json 1:event 1:util"
> > log_outputs="1:syslog:virtlogd"
> > max_age_days = 1
> > log_root = "/var/log/libvirt"
> > - I have also patched the cleaner timer with the patch attached so
> > that I could debug it quickly and not have to wait a full day :-)
> > - Create a couple of VMs so that some logs are created.
> >
> > - When you enable the above you could see on the logs the message:
> > "error : virLogCleanerParseFilename:89 : internal error: Failed to
> > parse rotated index from ''
>
>
> Ah, that helped. Thanks. IMO the proper fix is something among these lines:
>
> diff --git i/src/logging/log_cleaner.c w/src/logging/log_cleaner.c
> index 4ee91843aa..9f987b02b5 100644
> --- i/src/logging/log_cleaner.c
> +++ w/src/logging/log_cleaner.c
> @@ -82,10 +82,8 @@ virLogCleanerParseFilename(const char *path,
> *rotated_index = 0;
> rotated_index_str = g_match_info_fetch(matchInfo, 3);
>
> - if (!rotated_index_str)
> - return chain_prefix;
> -
> - if (virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
> + if (rotated_index_str && STRNEQ(rotated_index_str, "") &&
> + virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to parse rotated index from '%1$s'"),
> rotated_index_str);
>
> If the path lacks ".N" suffix, then the third match group is empty and
> g_match_info_fetch() returns an empty string. IOW, we should be parsing
> the suffix iff it's a non-empty string.
>
> Alternatively, we might use g_match_info_get_match_count() to find out
> how many groups were matched.
> What I also experimented with was passing G_REGEX_MATCH_NOTEMPTY flag to
> g_regex_new() but that didn't work. Idea was to make
> g_match_info_fetch(,3) return NULL, but I have no idea why it didn't
> work.
>
> Can you please send v2?
>
> Michal
>
--
David Negreira
Senior Sustaining Operations Engineer | Canonical
e: david.negreira@xxxxxxxxxxxxx
w: www.ubuntu.com | Support: support.canonical.com
