[syslog-ng] [PATCH] basicfuncs: Implement a $(substr STRING START LEN) template function.

[Balazs Scheidler]

Hi,

I would want to integrate this into 3.4, since I've opened it. Could one
of you please prepare a combined patch, that contains both Gergely's and
Jakub's work?

Thanks.

On Wed, 2011-04-13 at 16:13 +0200, Gergely Nagy wrote:
> Jakub Jankowski <shasta at toxcorp.com> writes:
> 
> > Tuesday 12 of April 2011 10:50:05 Jakub Jankowski napisał(a):
> >> Saturday 09 of April 2011 13:05:38 Gergely Nagy napisał(a):
> >> > This implements the $(substr) template function - a fairly primitive
> >> > one: it does error checking, but there's no error reporting present,
> >> > therefore invalid function calls will just get ignored.
> >>
> >> Thanks! I'll be testing this later this week and report back issues (if
> >> any).
> >
> > This is what I came up with, based on your patch:
> >  - it changes arguments order to $(substr <string> <offsetstart> [length]) - so
> >    string is first
> >  - offset = 0 means beginning of the string (and not 1)
> >  - called without third (length) argument returns substring from desired start 
> >    offset to the end of the string
> >  - works with negative offsets/lengths in a manner similar to other implementations:
> >    - negative offset means "start that many characters counting from the 
> >      end of the string", with -1 being the last character
> >    - negative length means "return substring from desired start up to that many 
> >      characters from the end of the string"
> >  - adds some msg_error()s on argument parsing failures
> 
> All of these sound good and reasonable, thank you!
> 
> > I'm not entirely sure if there are no off-by-one (or even off-by-many :) errors,
> > I'd be grateful if someone could check that. I also had issues comparing negative
> > glongs (signed) with GString's structure len field, which is gsize (so unsigned),
> > hence that ugly type change at the beginning. Can anyone point me to a *right*
> > direction there?
> 
> I think you're on the right track there, but see my comments on the
> patch itself.
> 
> > +static void
> > +tf_substr(LogMessage *msg, gint argc, GString *argv[], GString *result)
> > +{
> > +  glong start, len;
> > +  glong argv0len;
> > +
> > +  /*
> > +   * We need to cast argv[0]->len, which is gsize (so unsigned) type, to a
> > +   * signed type, so we can compare negative offsets/lengths with string
> > +   * length. But if argv[0]->len is bigger than G_MAXLONG, this can lead to
> > +   * completely wrong calculations, so we'll just return nothing (alternative
> > +   * would be to return original string and perhaps print an error...)
> > +   */
> > +  if (argv[0]->len >= G_MAXLONG) {
> > +    /* msg_error("$(substr) error: string length is too big to fit into glong type", NULL); */
> > +    return;
> > +  }
> > +  /* K&R probably hate me */
> > +  argv0len = argv[0]->len;
> > +
> 
> The intent here is good, but it could - in my opinion - be made a little
> more efficient:
> 
> if (argv[0]->len >= G_MAXLONG) {
>   msg_error("$(substr) error: string is too long", NULL);
>   return;
> }
> 
> And then just cast argv[0]->len to glong if so need be (see later).
> 
> I'd say the error message can be simple too, mentioning the exact type
> is in my opinion, not needed. For those who'd understand what it means,
> it doesn't add much extra value, as they can just look at the
> source. For others, it's confusing.
> 
> Short & to the point.
> 
> Also, if we return due to a string being too long, I believe we should
> print at least a warning (but an error seems more
> appropriate). Returning the original string in this case sounds
> non-intuitive, so - as you did in your patch - it's better to do nothing
> instead.
> 
> > +  /* if we were called with >2 arguments, third was desired length */
> > +  if (argc > 2) {
> > +    if (!tf_substr_parse_int(argv[2]->str, &len)) {
> > +      msg_error("$(substr) parsing failed, length could not be parsed", evt_tag_str("length", argv[2]->str), NULL);
> > +      return;
> > +    }
> > +  } else
> > +    len = argv0len;
> 
> len = (glong)argv[0]->len
> 
> And then you saved a variable, accomplished the same thing, and noone
> hates you! :]
> 
> > +  /* 
> > +   * if requested substring length is negative and beyond string start, do nothing;
> > +   * also if it is greater than string length, limit it to string length
> > +   */
> > +  if (len < 0 && -(len) > argv0len)
> > +    return;
> > +  else if (len > argv0len)
> > +    len = argv0len;
> 
> See above. You can cast argv[0]->len to glong within the if, to avoid
> compiler warnings - and it's safe too, since you checked the length at
> the top.
> 
> This goes for the rest of the len<->argv0len stuff too. However,
> explicit casting is merely my preference, using a variable is perfectly
> good too. Just name it something different then, like... "stringlen" -
> so one reading the code will have an idea what it is, without having to
> scroll up and check how it is initialized.
> 
> Other than these, your patch looks fine on the first read, good work!
> 
> I'll have another look at it as soon as possible, and apply it to my 3.3
> branch too.
> 

-- 
Bazsi