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