Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
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(a)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.