Daniel P. Berrangé writes:
> Only expose the type safe getters/setters to other code in preparation
> for changing the internal storage of data.
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
> src/libvirt_private.syms | 2 --
> src/util/viridentity.c | 28 ++++++++++++++++-----
> src/util/viridentity.h | 25 -------------------
> tests/viridentitytest.c | 45 +++++++++-------------------------
> tests/virnetserverclienttest.c | 45 +++++++++++++++-------------------
> 5 files changed, 54 insertions(+), 91 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ac357583e4..c7fb8c94e4 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2136,7 +2136,6 @@ virHostMemSetParameters;
>
>
> # util/viridentity.h
> -virIdentityGetAttr;
> virIdentityGetCurrent;
> virIdentityGetOSGroupID;
> virIdentityGetOSGroupName;
> @@ -2150,7 +2149,6 @@ virIdentityGetSystem;
> virIdentityGetX509DName;
> virIdentityIsEqual;
> virIdentityNew;
> -virIdentitySetAttr;
> virIdentitySetCurrent;
> virIdentitySetOSGroupID;
> virIdentitySetOSGroupName;
> diff --git a/src/util/viridentity.c b/src/util/viridentity.c
> index 2c6c0ee91f..fe0c416bba 100644
> --- a/src/util/viridentity.c
> +++ b/src/util/viridentity.c
> @@ -41,6 +41,20 @@
>
> VIR_LOG_INIT("util.identity");
>
> +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?
> +
> struct _virIdentity {
> virObject parent;
>
> @@ -233,9 +247,10 @@ static void virIdentityDispose(void *object)
> *
> * Returns: 0 on success, or -1 on error
> */
> -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.
> + const char *value)
> {
> int ret = -1;
> VIR_DEBUG("ident=%p attribute=%u value=%s", ident, attr, value);
> @@ -269,9 +284,10 @@ int virIdentitySetAttr(virIdentityPtr ident,
> *
> * Returns 0 on success, -1 on error
> */
> -int virIdentityGetAttr(virIdentityPtr ident,
> - unsigned int attr,
> - const char **value)
> +static int
> +virIdentityGetAttr(virIdentityPtr ident,
> + unsigned int attr,
virIdentityAttrType
> + const char **value)
> {
> VIR_DEBUG("ident=%p attribute=%d value=%p", ident, attr, value);
>
> diff --git a/src/util/viridentity.h b/src/util/viridentity.h
> index 4b87506373..0925b740d9 100644
> --- a/src/util/viridentity.h
> +++ b/src/util/viridentity.h
> @@ -26,20 +26,6 @@
> typedef struct _virIdentity virIdentity;
> typedef virIdentity *virIdentityPtr;
>
> -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;
> -
> virIdentityPtr virIdentityGetCurrent(void);
> int virIdentitySetCurrent(virIdentityPtr ident);
>
> @@ -47,17 +33,6 @@ virIdentityPtr virIdentityGetSystem(void);
>
> virIdentityPtr virIdentityNew(void);
>
> -int virIdentitySetAttr(virIdentityPtr ident,
> - unsigned int attr,
> - const char *value)
> - ATTRIBUTE_NONNULL(1)
> - ATTRIBUTE_NONNULL(3);
> -
> -int virIdentityGetAttr(virIdentityPtr ident,
> - unsigned int attr,
> - const char **value)
> - ATTRIBUTE_NONNULL(1)
> - ATTRIBUTE_NONNULL(3);
>
> bool virIdentityIsEqual(virIdentityPtr identA,
> virIdentityPtr identB)
> diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c
> index 64b511c272..e57b68ec43 100644
> --- a/tests/viridentitytest.c
> +++ b/tests/viridentitytest.c
> @@ -45,14 +45,11 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED)
> if (!(ident = virIdentityNew()))
> goto cleanup;
>
> - 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...
> goto cleanup;
>
> - if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_USER_NAME,
> - &val) < 0)
> + if (virIdentityGetOSUserName(ident,
> + &val) < 0)
> goto cleanup;
>
> if (STRNEQ_NULLABLE(val, "fred")) {
> @@ -60,9 +57,7 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> - if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_GROUP_NAME,
> - &val) < 0)
> + if (virIdentityGetOSGroupName(ident, &val) < 0)
> goto cleanup;
>
> if (val != NULL) {
> @@ -70,16 +65,12 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> - 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.
>
> - if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_USER_NAME,
> - &val) < 0)
> + if (virIdentityGetOSUserName(ident, &val) < 0)
> goto cleanup;
>
> if (STRNEQ_NULLABLE(val, "fred")) {
> @@ -110,9 +101,7 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> - if (virIdentitySetAttr(identa,
> - VIR_IDENTITY_ATTR_OS_USER_NAME,
> - "fred") < 0)
> + if (virIdentitySetOSUserName(identa, "fred") < 0)
> goto cleanup;
>
> if (virIdentityIsEqual(identa, identb)) {
> @@ -120,9 +109,7 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> - if (virIdentitySetAttr(identb,
> - VIR_IDENTITY_ATTR_OS_USER_NAME,
> - "fred") < 0)
> + if (virIdentitySetOSUserName(identb, "fred") < 0)
> goto cleanup;
>
> if (!virIdentityIsEqual(identa, identb)) {
> @@ -130,13 +117,9 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> - if (virIdentitySetAttr(identa,
> - VIR_IDENTITY_ATTR_OS_GROUP_NAME,
> - "flintstone") < 0)
> + if (virIdentitySetOSGroupName(identa, "flintstone") < 0)
> goto cleanup;
> - if (virIdentitySetAttr(identb,
> - VIR_IDENTITY_ATTR_OS_GROUP_NAME,
> - "flintstone") < 0)
> + if (virIdentitySetOSGroupName(identb, "flintstone") < 0)
> goto cleanup;
>
> if (!virIdentityIsEqual(identa, identb)) {
> @@ -144,9 +127,7 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> - if (virIdentitySetAttr(identb,
> - VIR_IDENTITY_ATTR_SASL_USER_NAME,
> - "fred@xxxxxxxxxxxxxx") < 0)
> + if (virIdentitySetSASLUserName(identb, "fred@xxxxxxxxxxxxxx") < 0)
> goto cleanup;
>
> if (virIdentityIsEqual(identa, identb)) {
> @@ -181,9 +162,7 @@ static int testIdentityGetSystem(const void *data)
> goto cleanup;
> }
>
> - if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_SELINUX_CONTEXT,
> - &val) < 0)
> + if (virIdentityGetSELinuxContext(ident, &val) < 0)
> goto cleanup;
>
> if (STRNEQ_NULLABLE(val, context)) {
> diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c
> index 280bd24227..603afadab4 100644
> --- a/tests/virnetserverclienttest.c
> +++ b/tests/virnetserverclienttest.c
> @@ -53,9 +53,9 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
> virNetServerClientPtr client = NULL;
> virIdentityPtr ident = NULL;
> const char *gotUsername = NULL;
> - const char *gotUserID = NULL;
> + uid_t gotUserID;
> const char *gotGroupname = NULL;
> - const char *gotGroupID = NULL;
> + gid_t gotGroupID;
> const char *gotSELinuxContext = NULL;
>
> if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) < 0) {
> @@ -85,9 +85,8 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> - 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.
> fprintf(stderr, "Missing username in identity\n");
> goto cleanup;
> }
> @@ -97,21 +96,19 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> - if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_USER_ID,
> - &gotUserID) < 0) {
> + if (virIdentityGetOSUserID(ident,
> + &gotUserID) < 0) {
Would fit on a single line
> fprintf(stderr, "Missing user ID in identity\n");
> goto cleanup;
> }
> - if (STRNEQ_NULLABLE("666", gotUserID)) {
> - fprintf(stderr, "Want username '666' got '%s'\n",
> - NULLSTR(gotUserID));
> + if (666 != gotUserID) {
> + fprintf(stderr, "Want username '666' got '%llu'\n",
> + (unsigned long long)gotUserID);
> goto cleanup;
> }
>
> - if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_GROUP_NAME,
> - &gotGroupname) < 0) {
> + if (virIdentityGetOSGroupName(ident,
> + &gotGroupname) < 0) {
> fprintf(stderr, "Missing groupname in identity\n");
> goto cleanup;
> }
> @@ -121,27 +118,25 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> - if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_GROUP_ID,
> - &gotGroupID) < 0) {
> + if (virIdentityGetOSGroupID(ident,
> + &gotGroupID) < 0) {
> fprintf(stderr, "Missing group ID in identity\n");
> goto cleanup;
> }
> - if (STRNEQ_NULLABLE("7337", gotGroupID)) {
> - fprintf(stderr, "Want groupname '7337' got '%s'\n",
> - NULLSTR(gotGroupID));
> + if (7337 != gotGroupID) {
> + fprintf(stderr, "Want groupname '7337' got '%llu'\n",
> + (unsigned long long)gotGroupID);
> goto cleanup;
> }
>
> - if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_SELINUX_CONTEXT,
> - &gotSELinuxContext) < 0) {
> + if (virIdentityGetSELinuxContext(ident,
> + &gotSELinuxContext) < 0) {
> fprintf(stderr, "Missing SELinux context in identity\n");
> goto cleanup;
> }
> 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.)
Only minor nits, nothing functional.
Reviewed-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> }
>
> --
> 2.21.0
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
