1. Patchew
  2. Libvirt
  3. View series

[PATCH] fix virLogCleanerParseFilename logic

David Negreira posted 1 patch 6 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20240519154036.104016-1-david.negreira@canonical.com
src/logging/log_cleaner.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] fix virLogCleanerParseFilename logic
Posted by David Negreira 6 months, 1 week ago 
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
Re: [PATCH] fix virLogCleanerParseFilename logic
Posted by Michal Prívozník 6 months ago 
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
Re: [PATCH] fix virLogCleanerParseFilename logic
Posted by David Negreira 6 months ago 
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
Re: [PATCH] fix virLogCleanerParseFilename logic
Posted by Michal Prívozník 6 months ago 
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
Re: [PATCH] fix virLogCleanerParseFilename logic
Posted by David Negreira 6 months ago 
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
Re: [PATCH] fix virLogCleanerParseFilename logic
Posted by David Negreira 5 months, 4 weeks ago 
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
Re: [PATCH] fix virLogCleanerParseFilename logic
Posted by Michal Prívozník 5 months, 4 weeks ago 
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.

Michal
Re: [PATCH] fix virLogCleanerParseFilename logic
Posted by Michal Prívozník 5 months, 4 weeks ago 
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/ZWZ7GFFLZTCMWDUUB3X4FFK4YT5LW4OA/

Michal
Re: [PATCH] fix virLogCleanerParseFilename logic
Posted by David Negreira 5 months, 4 weeks ago 
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/ZWZ7GFFLZTCMWDUUB3X4FFK4YT5LW4OA/
>
> Michal
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.