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@xxxxxxxxxx> 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.
