Re: [FFmpeg-devel] [PATCH v2 1/7] avutil/uuid: add utility library for manipulating UUIDs as specified in RFC 4122

[Pierre-Anthony Lemieux <pal@sandflow.com>]

On Wed, May 11, 2022 at 8:02 AM Zane van Iperen <zane@zanevaniperen.com> wrote:
>
>
>
> On 10/5/22 23:18, Andreas Rheinhardt wrote:
>
> >> +int av_uuid_parse_range(const char *in_start, const char *in_end, AVUUID uu)
> >> +{
> >> +    int i;
> >> +    const char *cp;
> >> +    char buf[3];
> >> +
> >> +    if ((in_end - in_start) != 36)
> >> +        return -1;
> >> +
> >> +    for (i = 0, cp = in_start; i < 36; i++, cp++) {
> >> +        if (i == 8 || i == 13 || i == 18 || i == 23) {
> >> +            if (*cp == '-')
> >> +                continue;
> >> +            return AVERROR(EINVAL);
> >> +        }
> >> +
> >> +        if (!av_isxdigit(*cp))
> >> +            return AVERROR(EINVAL);
> >> +    }
> >> +
> >> +    buf[2] = '\0';
> >> +    for (i = 0, cp = in_start; i < 16; i++) {
> >> +        if (i == 4 || i == 6 || i == 8 || i == 10)
> >> +            cp++;
> >> +
> >> +        buf[0] = *cp++;
> >> +        buf[1] = *cp++;
> >> +
> >> +        errno = 0;
> >> +        uu[i] = strtoul(buf, NULL, 16);
> >> +        if (errno)
> >> +            return AVERROR(errno);
> >
> > How could this ever happen given that you have already checked that the
> > buffer only contains hex digits? And isn't using strtoul a bit overkill
> > anyway? I'd just check and parse this stuff in one loop.
> >
>
> Yeah, good point. It's based off libuuid's, which has some time/clock uuid handling
> between the two loops. I'll tidy it up in the next few days... hopefully...
>
> >> +/**
> >> + * @file
> >> + * UUID parsing and serialization utilities.
> >> + * The library treat the UUID as an opaque sequence of 16 unsigned bytes,
> >                         ^
> >                         s
>
> Fixed.
>
> >> +/**
> >> + * Parses a string representation of a UUID formatted according to IETF RFC 4122
> >> + * into an AVUUID. The parsing is case-insensitive. The string must be 37
> >> + * characters long, including the terminating NULL character.
> >
> > NUL, NULL is for pointers.
> >
>
> Changed.
>
> >> +/**
> >> + * Parses a string representation of a UUID formatted according to IETF RFC 4122
> >> + * into an AVUUID. The parsing is case-insensitive. The string must consist of
> >> + * 36 characters, i.e. `in_end - in_start == 36`
> >> + *
> >> + * @param[in]  in_start Pointer to the first character of the string representation
> >> + * @param[in]  in_end   Pointer to the character after the last character of the
> >> + *                      string representation. That memory location is never
> >> + *                      accessed
> >> + * @param[out] uu       AVUUID
> >> + * @return              A non-zero value in case of an error.
> >> + */
> >> +int av_uuid_parse_range(const char *in_start, const char *in_end, AVUUID uu);
> >
> > This sounds like in_end is actually redundant. Does retaining it improve
> > extensibility?
> >
>
> I believe so. The main difference is av_uuid_parse_range() can handle non NUL-termiated
> strings. I can just remove the entire last sentence (or change "must" to "should").

I have modified the documentation in v3 of the patchset to emphasize
that `in_end - in_start != 36` results in an error.

>
> >> +
> >> +/**
> >> + * Serializes a AVUUID into a string representation according to IETF RFC 4122.
> >> + * The string is lowercase and always 37 characters long, including the
> >> + * terminating NULL character.
> >> + *
> >> + * @param[in]  uu  AVUUID
> >> + * @param[out] out Pointer to a array of no less than 37 characters.
> >                                    ^
> >                                    n
> >
>
> Fixed.
>
>
> >> +/**
> >> + * Sets a UUID to nil
> >> + *
> >> + * @param[in,out]  uu  UUID to be set to nil
> >> + */
> >> +void av_uuid_nil_set(AVUUID uu);
> >
> > Why are these three functions not static inline? Exporting them seems
> > like a waste.
> >
>
> No particular reason, it's easy enough to change.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".