[libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser

[Dmitrii Shcherbakov]

Thanks a lot for the review!

Responses inline - ACKs to address in v6.

On Fri, Oct 1, 2021 at 12:58 PM Daniel P. Berrangé <berrange at redhat.com> wrote:
> I don't  know what the thread concurrency rules are of the callers of
> this code, but regardless we generally aim to make any one-time
> initializers thread safe by default.
>
> Recommend using VIR_ONCE_GLOBAL_INIT to do one-time init.
>

Ack, will address in v6.

> IIUC, this hash table is created once and never changed. I'm
> not seeing a clear need for g_strdup here. Can't we just
> directly use the constant string as the key ?

Good point, no need to store copies of constant strings here.

> > +    if (!g_regex_match_simple("^[a-zA-Z0-9\\-_\\s,.:=]*$", value, 0, 0)) {
>
> Since you're only trying to validate a set of permitted characters,
> it is sufficient to use strspn() for this task.
>
> eg what we do in vsh.c is
>
>   /* Characters permitted in $EDITOR environment variable and temp filename. */
>   #define ACCEPTED_CHARS \
>     "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-/_.:@"
>
>     if (strspn(editor, ACCEPTED_CHARS) != strlen(editor)) {
>        ....error...

Ack, seems like a cheaper way to do this than with regex.

> > +G_BEGIN_DECLS;
>
> This is not required in libvirt internal code, since we never use C++
> internally.

Ack, will remove in v6.

> > +    if (ftruncate(fd, len) != 0) {
> > +        VIR_TEST_DEBUG("%s: %s", "failed to ftruncate an anonymous file",
> > +                g_strerror(errno));
> > +        goto cleanup;
> > +    }
>
> Since it is a new temporary file it is guaranteed zero length, so
> this should be redundant AFAICT.

Ack, will address in v6.