Re: [PATCH] fix virLogCleanerParseFilename logic

[David Negreira <david.negreira@xxxxxxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxx
w: www.ubuntu.com | Support: support.canonical.com

diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c
index d8e6ce9cdd..413df54eb9 100644
--- a/src/logging/log_cleaner.c
+++ b/src/logging/log_cleaner.c
@@ -39,7 +39,7 @@ VIR_LOG_INIT("logging.log_cleaner");
 
 /* Cleanup log root (/var/log/libvirt) and all subfolders (e.g. /var/log/libvirt/qemu) */
 #define CLEANER_LOG_DEPTH 1
-#define CLEANER_LOG_TIMEOUT_MS (24 * 3600 * 1000) /* One day */
+#define CLEANER_LOG_TIMEOUT_MS (1 * 60 * 1000) /* 1 MINUTE */
 #define MAX_TIME ((time_t) G_MAXINT64)
 
 static GRegex *log_regex;