[PATCH] fix virLogCleanerParseFilename logic
We should return the full filename path when we don't have a match on the third group of the regex. Signed-off-by: David Negreira <david.negreira@canonical.com> --- src/logging/log_cleaner.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c index 4ee91843aa..d8e6ce9cdd 100644 --- a/src/logging/log_cleaner.c +++ b/src/logging/log_cleaner.c @@ -82,7 +82,7 @@ virLogCleanerParseFilename(const char *path, *rotated_index = 0; rotated_index_str = g_match_info_fetch(matchInfo, 3); - if (!rotated_index_str) + if (rotated_index_str) return chain_prefix; if (virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) { -- 2.40.1
On 5/19/24 17:40, David Negreira wrote:
We should return the full filename path when we don't have a match on the third group of the regex.
Signed-off-by: David Negreira <david.negreira@canonical.com> --- src/logging/log_cleaner.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c index 4ee91843aa..d8e6ce9cdd 100644 --- a/src/logging/log_cleaner.c +++ b/src/logging/log_cleaner.c @@ -82,7 +82,7 @@ virLogCleanerParseFilename(const char *path, *rotated_index = 0; rotated_index_str = g_match_info_fetch(matchInfo, 3);
- if (!rotated_index_str) + if (rotated_index_str) return chain_prefix;
if (virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
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
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 '' Kind regards, David Negreira On Tue, May 21, 2024 at 10:42 AM Michal Prívozník <mprivozn@redhat.com> wrote:
On 5/19/24 17:40, David Negreira wrote:
We should return the full filename path when we don't have a match on the third group of the regex.
Signed-off-by: David Negreira <david.negreira@canonical.com> --- src/logging/log_cleaner.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c index 4ee91843aa..d8e6ce9cdd 100644 --- a/src/logging/log_cleaner.c +++ b/src/logging/log_cleaner.c @@ -82,7 +82,7 @@ virLogCleanerParseFilename(const char *path, *rotated_index = 0; rotated_index_str = g_match_info_fetch(matchInfo, 3);
- if (!rotated_index_str) + if (rotated_index_str) return chain_prefix;
if (virStrToLong_i(rotated_index_str, NULL, 10, rotated_index) < 0) {
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
-- David Negreira Senior Sustaining Operations Engineer | Canonical e: david.negreira@canonical.com w: www.ubuntu.com | Support: support.canonical.com
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
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
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
On 5/29/24 09:21, Michal Prívozník wrote:
On 5/29/24 09:04, David Negreira wrote:
Gentle reminder
Can you please send v2?
Michal
Oh, I thought you were going to send v2. But okay, let me do that.
Done: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/ZWZ7G... Michal
Appreciated Michal, thank you for your valuable input and help. On Wed, May 29, 2024 at 9:52 AM Michal Prívozník <mprivozn@redhat.com> wrote:
On 5/29/24 09:21, Michal Prívozník wrote:
On 5/29/24 09:04, David Negreira wrote:
Gentle reminder
Can you please send v2?
Michal
Oh, I thought you were going to send v2. But okay, let me do that.
Done:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/ZWZ7G...
Michal
participants (2)
-
David Negreira -
Michal Prívozník