[libvirt] [RFC][PATCH] storage: allow alphabetical names in owner/group of permissions

Hi, This patch allows to specify owner/group of permissions in storage XML as alphabetical names. One concern of this patch is since alphabetical names are converted to uid/gid at XML parsing (e.g., virsh pool-define some-pool.xml), the generated XML file for libvirt internal does not contain the original alphabetical names and dumpxml does not as well. Is it better the conversion is done when activated not defined? And I know I need to modify documents and write test XMLs. Any comments are welcome! Thanks, ozaki-r
From bf8a01689e345ec9c08a1348b5781c51b6016ef4 Mon Sep 17 00:00:00 2001 From: Ryota Ozaki <ozaki.ryota@gmail.com> Date: Sun, 15 Mar 2009 02:30:02 +0900 Subject: [PATCH] storage: allow alphabetical names in owner/group of permissions
--- configure.in | 2 +- src/storage_conf.c | 50 ++++++++++++++++++++++++++++++++++++--------- src/util.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++- src/util.h | 8 +++++++ 4 files changed, 105 insertions(+), 12 deletions(-) diff --git a/configure.in b/configure.in index 413d27c..0277d9a 100644 --- a/configure.in +++ b/configure.in @@ -75,7 +75,7 @@ dnl Availability of various common functions (non-fatal if missing). AC_CHECK_FUNCS([cfmakeraw regexec uname sched_getaffinity getuid getgid]) dnl Availability of various not common threadsafe functions -AC_CHECK_FUNCS([strerror_r strtok_r getmntent_r getgrnam_r getpwuid_r]) +AC_CHECK_FUNCS([strerror_r strtok_r getmntent_r getgrnam_r getpwuid_r getpwnam_r]) dnl Availability of various common headers (non-fatal if missing). AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/syslimits.h sys/utsname.h sys/wait.h winsock2.h sched.h termios.h sys/poll.h syslog.h]) diff --git a/src/storage_conf.c b/src/storage_conf.c index 1c9a4e5..7d75b71 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -412,23 +412,53 @@ virStorageDefParsePerms(virConnectPtr conn, if (virXPathNode(conn, "./owner", ctxt) == NULL) { perms->uid = getuid(); } else { - if (virXPathLong(conn, "number(./owner)", ctxt, &v) < 0) { - virStorageReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("malformed owner element")); - goto error; + if (virXPathLong(conn, "number(./owner)", ctxt, &v) == 0) { + perms->uid = (int)v; + } else { + char *name; + int uid; + + name = virXPathString(conn, "string(./owner)", ctxt); + if (!name) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("malformed owner element")); + goto error; + } + + uid = virGetUIDByUsername(conn, name); + VIR_FREE(name); + if (uid < 0) { + goto error; + } + + perms->uid = uid; } - perms->uid = (int)v; } if (virXPathNode(conn, "./group", ctxt) == NULL) { perms->gid = getgid(); } else { - if (virXPathLong(conn, "number(./group)", ctxt, &v) < 0) { - virStorageReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("malformed group element")); - goto error; + if (virXPathLong(conn, "number(./group)", ctxt, &v) == 0) { + perms->gid = (int)v; + } else { + char *name; + int gid; + + name = virXPathString(conn, "string(./group)", ctxt); + if (!name) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("malformed group element")); + goto error; + } + + gid = virGetGIDByGroupname(conn, name); + VIR_FREE(name); + if (gid < 0) { + goto error; + } + + perms->gid = gid; } - perms->gid = (int)v; } /* NB, we're ignoring missing labels here - they'll simply inherit */ diff --git a/src/util.c b/src/util.c index 9b74757..fbe329d 100644 --- a/src/util.c +++ b/src/util.c @@ -50,9 +50,12 @@ #include <paths.h> #endif #include <netdb.h> -#ifdef HAVE_GETPWUID_R +#if defined(HAVE_GETPWUID_R) || defined(HAVE_GETPWNAM_R) #include <pwd.h> #endif +#ifdef HAVE_GETGRNAM_R +#include <grp.h> +#endif #include "virterror_internal.h" #include "logging.h" @@ -1558,3 +1561,55 @@ char *virGetUserDirectory(virConnectPtr conn, return ret; } #endif + + +#ifdef HAVE_GETPWNAM_R +int virGetUIDByUsername(virConnectPtr conn, char *name) +{ + struct passwd pwbuf; + struct passwd *pw = NULL; + char *strbuf; + size_t strbuflen = sysconf(_SC_GETPW_R_SIZE_MAX); + + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { + virReportOOMError(conn); + return -1; + } + + if (getpwnam_r(name, &pwbuf, strbuf, strbuflen, &pw) != 0 || pw == NULL) { + virReportSystemError(conn, errno, + _("Failed to find user record for name '%s'"), + name); + VIR_FREE(strbuf); + return -1; + } + VIR_FREE(strbuf); + return pw->pw_uid; +} +#endif + + +#ifdef HAVE_GETGRNAM_R +int virGetGIDByGroupname(virConnectPtr conn, char *name) +{ + struct group grbuf; + struct group *gr = NULL; + char *strbuf; + size_t strbuflen = sysconf(_SC_GETGR_R_SIZE_MAX); + + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { + virReportOOMError(conn); + return -1; + } + + if (getgrnam_r(name, &grbuf, strbuf, strbuflen, &gr) != 0 || gr == NULL) { + virReportSystemError(conn, errno, + _("Failed to find group record for name '%s'"), + name); + VIR_FREE(strbuf); + return -1; + } + VIR_FREE(strbuf); + return gr->gr_gid; +} +#endif diff --git a/src/util.h b/src/util.h index 87cbf67..ffd3458 100644 --- a/src/util.h +++ b/src/util.h @@ -195,6 +195,14 @@ char *virGetUserDirectory(virConnectPtr conn, uid_t uid); #endif +#ifdef HAVE_GETPWNAM_R +int virGetUIDByUsername(virConnectPtr conn, char *name); +#endif + +#ifdef HAVE_GETGRNAM_R +int virGetGIDByGroupname(virConnectPtr conn, char *name); +#endif + int virRandomInitialize(unsigned int seed); int virRandom(int max); -- 1.6.0.6

On Sun, Mar 15, 2009 at 02:47:14AM +0900, Ryota Ozaki wrote:
Hi,
This patch allows to specify owner/group of permissions in storage XML as alphabetical names.
One concern of this patch is since alphabetical names are converted to uid/gid at XML parsing (e.g., virsh pool-define some-pool.xml), the generated XML file for libvirt internal does not contain the original alphabetical names and dumpxml does not as well. Is it better the conversion is done when activated not defined?
Yes, I'm wondering about this. Usually it's better if the XML can round-trip without change (i.e. the exported version is the same as the imported one), modulo maybe some reordering of elements. But loosing information or distortions in the cycle tend to generate problems. So I'm wondering what is in your opinion the added value of having livirt do that conversion instead of the layer on top of libvirt. There is the added problem that the given name might not be portable from one node to another, that's true too for uid/gid values but for example in the case of networked filesystems, the hardcoded numbers are actually more portable than the high level name equivalents. So I'm not sure really if the advantage really outweight clearly the potential problems. I don't know, it's probably a matter of use cases. Otherwise the patch looks fine to me, test case may be harder due to the non-portability of the names. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Hi Daniel, On Mon, Mar 16, 2009 at 6:01 PM, Daniel Veillard <veillard@redhat.com> wrote:
On Sun, Mar 15, 2009 at 02:47:14AM +0900, Ryota Ozaki wrote:
Hi,
This patch allows to specify owner/group of permissions in storage XML as alphabetical names.
One concern of this patch is since alphabetical names are converted to uid/gid at XML parsing (e.g., virsh pool-define some-pool.xml), the generated XML file for libvirt internal does not contain the original alphabetical names and dumpxml does not as well. Is it better the conversion is done when activated not defined?
Yes, I'm wondering about this. Usually it's better if the XML can round-trip without change (i.e. the exported version is the same as the imported one), modulo maybe some reordering of elements. But loosing information or distortions in the cycle tend to generate problems. So I'm wondering what is in your opinion the added value of having livirt do that conversion instead of the layer on top of libvirt.
The reason why I want this function is just ease of use because we usually specify owner/group by names not numbers, but I have no objection to your concern of loosing information. So I will revise the patch to fix the problem. And the reason I did odd implementation is that I worried about the impact by modifying the internal data structure (apologies I didn't tell this). How about this point?
There is the added problem that the given name might not be portable from one node to another, that's true too for uid/gid values but for example in the case of networked filesystems, the hardcoded numbers are actually more portable than the high level name equivalents.
I thought same thing, but I think it's probably not matter. In many cases, user accounts would be centrally managed, and if not so, hardcoded numbers also don't make sense because uid/gid values make sense only if the corresponding accounts exist. So IMO the portability thing should be considered in operations.
So I'm not sure really if the advantage really outweight clearly the potential problems. I don't know, it's probably a matter of use cases.
Anyway, I'm sure I will remain the capability to specify with numeric numbers, so there should be no regression ;-)
Otherwise the patch looks fine to me, test case may be harder due to the non-portability of the names.
Thanks for reviewing! and I agree with you. If writing test cases, we need a trick, for example, creating a temporal account that has very higher unused uid/gid only for the tests. Thanks again, ozaki-r
Daniel
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Mon, Mar 16, 2009 at 10:01:38AM +0100, Daniel Veillard wrote:
On Sun, Mar 15, 2009 at 02:47:14AM +0900, Ryota Ozaki wrote:
Hi,
This patch allows to specify owner/group of permissions in storage XML as alphabetical names.
One concern of this patch is since alphabetical names are converted to uid/gid at XML parsing (e.g., virsh pool-define some-pool.xml), the generated XML file for libvirt internal does not contain the original alphabetical names and dumpxml does not as well. Is it better the conversion is done when activated not defined?
Yes, I'm wondering about this. Usually it's better if the XML can round-trip without change (i.e. the exported version is the same as the imported one), modulo maybe some reordering of elements. But loosing information or distortions in the cycle tend to generate problems. So I'm wondering what is in your opinion the added value of having livirt do that conversion instead of the layer on top of libvirt.
Yeah, I don't hugely like the idea of usernames turning into UIDs when you round-trip the XML. Other places where we didn't correctly preserve data on a round-trip have proved to be rather painful to deal with for apps (eg, automatic VNC port number, or automatic VIF names). I'm not sure what the right answer is for this problem of uid vs username. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange
-
Daniel Veillard
-
Ryota Ozaki