On 5/19/24 17:40, David Negreira wrote: I'm not sure this is the right fix. If rotated_index_str is NOT NULL chain_prefix is returned. Fair enough. But when it is NULL then it's passed to virStrToLong_i() which does not seem right. Also, do you have a minimalist reproducer? Michal

On 5/21/24 11:11, David Negreira wrote: 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

Gentle reminder On Thu, May 23, 2024 at 9:05 AM David Negreira <david.negreira(a)canonical.com&gt; wrote: > > 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(a)redhat.com&gt; 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(a)canonical.com > w: www.ubuntu.com | Support: support.canonical.com