Re: [PATCH] fix virLogCleanerParseFilename logic
Gentle reminder
On Thu, May 23, 2024 at 9:05 AM David Negreira
<david.negreira(a)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(a)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(a)canonical.com
> w: www.ubuntu.com | Support: support.canonical.com