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