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

[Zane van Iperen]

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").

>> +
>> +/**
>> + * 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.