Re: [libvirt] [PATCH v3 45/48] util: make generic identity accessors private
On Tue, 2019-07-30 at 16:47 +0200, Christophe de Dinechin wrote:
Daniel P. Berrangé writes:
> +++ b/src/util/viridentity.c
> +typedef enum {
> + VIR_IDENTITY_ATTR_OS_USER_NAME,
> + VIR_IDENTITY_ATTR_OS_USER_ID,
> + VIR_IDENTITY_ATTR_OS_GROUP_NAME,
> + VIR_IDENTITY_ATTR_OS_GROUP_ID,
> + VIR_IDENTITY_ATTR_OS_PROCESS_ID,
> + VIR_IDENTITY_ATTR_OS_PROCESS_TIME,
> + VIR_IDENTITY_ATTR_SASL_USER_NAME,
> + VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME,
> + VIR_IDENTITY_ATTR_SELINUX_CONTEXT,
> +
> + VIR_IDENTITY_ATTR_LAST,
> +} virIdentityAttrType;
Why define a typedef if it's never used?
It's just good manners :) Especially when you're just moving an
existing definition around.
> @@ -233,9 +247,10 @@ static void virIdentityDispose(void
*object)
> -int virIdentitySetAttr(virIdentityPtr ident,
> - unsigned int attr,
> - const char *value)
> +static int
> +virIdentitySetAttr(virIdentityPtr ident,
> + unsigned int attr,
e.g. here, might have virIdentityAttrType instead of unsigned int,
might help the compiler emit better diagnostics.
In some cases we're forced to use 'int' instead of the specific
enum because... Reasons. I forget :) But I dont think this is one of
those cases.
Considering that this patch is pure code motion (plus dealing with
the fallout of such code motion), it would not be very appropriate to
alter the function signature during the process. And anyway, none of
this really matters: we're gonna drop both the enum and the functions
in the next patch.
> +++ b/tests/viridentitytest.c
> @@ -45,14 +45,11 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED)
> - if (virIdentitySetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_USER_NAME,
> - "fred") < 0)
> + if (virIdentitySetOSUserName(ident, "fred") < 0)
(Following a discussion on another patch - Learning the conventions)
Here is a case were error is checked with < 0...
[...]
> @@ -70,16 +65,12 @@ static int testIdentityAttrs(const void
*data ATTRIBUTE_UNUSED)
> - if (virIdentitySetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_USER_NAME,
> - "joe") != -1) {
> + if (virIdentitySetOSUserName(ident, "joe") != -1) {
> VIR_DEBUG("Unexpectedly overwrote attribute");
> goto cleanup;
> }
... but the precise error is supposed to be -1 (at least when overwriting).
Not complaining, just taking notes.
I don't think the difference is intentional: just code written by
different developers and/or at different points in time. And again,
not something that it would be appropriate to change in this patch.
> @@ -85,9 +85,8 @@ static int testIdentity(const void *opaque
ATTRIBUTE_UNUSED)
> - if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_USER_NAME,
> - &gotUsername) < 0) {
> + if (virIdentityGetOSUserName(ident,
> + &gotUsername) < 0) {
I think this would fit on a single line now.
Agreed.
> @@ -121,27 +118,25 @@ static int testIdentity(const void *opaque
ATTRIBUTE_UNUSED)
> if (STRNEQ_NULLABLE("foo_u:bar_r:wizz_t:s0-s0:c0.c1023",
gotSELinuxContext)) {
> - fprintf(stderr, "Want groupname
'foo_u:bar_r:wizz_t:s0-s0:c0.c1023' got '%s'\n",
> - NULLSTR(gotGroupID));
> + fprintf(stderr, "Want SELinux context
'foo_u:bar_r:wizz_t:s0-s0:c0.c1023' got '%s'\n",
> + NULLSTR(gotSELinuxContext));
> goto cleanup;
That last fix not really related to the rest of the patch, but useful.
(I don't know how important it is in the libvirt community to not mix
"side fixes" with large patch series. I don't personally mind at all.)
It's usually frowned upon, though it's prefectly normal that
something will slip in, especially in the context of a fairly large
series such as this one.
Please split off that change to its own, trivial patch. With that
addressed,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization