[libvirt] [PATCH][take2][2/2] storage: allow alphabetical names in owner/group of permissions
Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
From c441d5f29f1ed964e3c17dcac8614c0834eaba49 Mon Sep 17 00:00:00 2001 From: Ryota Ozaki <ozaki.ryota@gmail.com> Date: Thu, 16 Apr 2009 23:17:29 +0900 Subject: [PATCH] storage: allow alphabetical names in owner/group of permissions
--- docs/schemas/storagepool.rng | 10 ++++- docs/schemas/storagevol.rng | 10 ++++- src/Makefile.am | 1 + src/storage_backend.c | 8 +++- src/storage_backend_fs.c | 32 ++++++++++++++- src/storage_backend_logical.c | 33 ++++++++++++++- src/storage_conf.c | 91 +++++++++++++++++++++++++++++------------ src/storage_conf.h | 4 +- 8 files changed, 152 insertions(+), 37 deletions(-) diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index e152e19..ed1e597 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -127,10 +127,16 @@ <ref name='uint'/> </element> <element name='owner'> - <ref name='uint'/> + <choice> + <ref name='uint'/> + <ref name='name'/> + </choice> </element> <element name='group'> - <ref name='uint'/> + <choice> + <ref name='uint'/> + <ref name='name'/> + </choice> </element> <optional> <element name='label'> diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index c7bd3a7..13e08b5 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -46,10 +46,16 @@ <ref name='uint'/> </element> <element name='owner'> - <ref name='uint'/> + <choice> + <ref name='uint'/> + <ref name='name'/> + </choice> </element> <element name='group'> - <ref name='uint'/> + <choice> + <ref name='uint'/> + <ref name='name'/> + </choice> </element> <optional> <element name='label'> diff --git a/src/Makefile.am b/src/Makefile.am index f176b46..84567aa 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -333,6 +333,7 @@ endif # Needed to keep automake quiet about conditionals libvirt_driver_storage_la_SOURCES = +libvirt_driver_storage_la_LIBADD = libvirt_util.la if WITH_STORAGE_DIR if WITH_DRIVER_MODULES mod_LTLIBRARIES += libvirt_driver_storage.la diff --git a/src/storage_backend.c b/src/storage_backend.c index b154140..810c4b5 100644 --- a/src/storage_backend.c +++ b/src/storage_backend.c @@ -214,8 +214,12 @@ virStorageBackendUpdateVolTargetInfoFD(virConnectPtr conn, } target->perms.mode = sb.st_mode & S_IRWXUGO; - target->perms.uid = sb.st_uid; - target->perms.gid = sb.st_gid; + VIR_FREE(target->perms.owner); + if (virAsprintf(&target->perms.owner, "%d", sb.st_uid) == -1) + return -1; + VIR_FREE(target->perms.group); + if (virAsprintf(&target->perms.group, "%d", sb.st_gid) == -1) + return -1; VIR_FREE(target->perms.label); diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index 5000f43..d48c278 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -1189,7 +1189,37 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, /* We can only chown/grp if root */ if (getuid() == 0) { - if (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0) { + int uid = 0, gid = 0; + char *owner = vol->target.perms.owner; + char *group = vol->target.perms.group; + + if (owner) { + char *end = NULL; + int id = strtol(owner, &end, 10); + if (*end || id < 0) { + id = virGetUIDByUsername(conn, owner); + if (id < 0) { + unlink(vol->target.path); + close(fd); + return -1; + } + } + uid = id; + } + if (group) { + char *end = NULL; + int id = strtol(group, &end, 10); + if (*end || id < 0) { + id = virGetUIDByUsername(conn, group); + if (id < 0) { + unlink(vol->target.path); + close(fd); + return -1; + } + } + gid = id; + } + if (fchown(fd, uid, gid) < 0) { virReportSystemError(conn, errno, _("cannot set file owner '%s'"), vol->target.path); diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c index cbd2765..7a869f6 100644 --- a/src/storage_backend_logical.c +++ b/src/storage_backend_logical.c @@ -615,7 +615,38 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, /* We can only chown/grp if root */ if (getuid() == 0) { - if (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0) { + int uid = 0, gid = 0; + char *owner = vol->target.perms.owner; + char *group = vol->target.perms.group; + + if (owner) { + char *end = NULL; + int id = strtol(owner, &end, 10); + if (*end || id < 0) { + id = virGetUIDByUsername(conn, owner); + if (id < 0) { + unlink(vol->target.path); + close(fd); + return -1; + } + } + uid = id; + } + if (group) { + char *end = NULL; + int id = strtol(group, &end, 10); + if (*end || id < 0) { + id = virGetUIDByUsername(conn, group); + if (id < 0) { + unlink(vol->target.path); + close(fd); + return -1; + } + } + gid = id; + } + + if (fchown(fd, uid, gid) < 0) { virReportSystemError(conn, errno, _("cannot set file owner '%s'"), vol->target.path); diff --git a/src/storage_conf.c b/src/storage_conf.c index 9e25ccb..fd34319 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -256,8 +256,12 @@ virStorageVolDefFree(virStorageVolDefPtr def) { VIR_FREE(def->source.extents); VIR_FREE(def->target.path); + VIR_FREE(def->target.perms.owner); + VIR_FREE(def->target.perms.group); VIR_FREE(def->target.perms.label); VIR_FREE(def->backingStore.path); + VIR_FREE(def->backingStore.perms.owner); + VIR_FREE(def->backingStore.perms.group); VIR_FREE(def->backingStore.perms.label); VIR_FREE(def); } @@ -295,6 +299,8 @@ virStoragePoolDefFree(virStoragePoolDefPtr def) { virStoragePoolSourceFree(&def->source); VIR_FREE(def->target.path); + VIR_FREE(def->target.perms.owner); + VIR_FREE(def->target.perms.group); VIR_FREE(def->target.perms.label); VIR_FREE(def); } @@ -391,15 +397,14 @@ virStorageDefParsePerms(virConnectPtr conn, xmlNodePtr relnode; xmlNodePtr node; + perms->mode = defaultmode; + perms->owner = NULL; + perms->group = NULL; + perms->label = NULL; + node = virXPathNode(conn, permxpath, ctxt); - if (node == NULL) { - /* Set default values if there is not <permissions> element */ - perms->mode = defaultmode; - perms->uid = getuid(); - perms->gid = getgid(); - perms->label = NULL; + if (node == NULL) return 0; - } relnode = ctxt->node; ctxt->node = node; @@ -420,25 +425,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")); + if (virAsprintf(&perms->owner, "%d", getuid()) == -1) { + ret = -ENOMEM; goto error; } - perms->uid = (int)v; + } else { + if (virXPathLong(conn, "number(./owner)", ctxt, &v) == 0) { + if (virAsprintf(&perms->owner, "%ld", v) == -1) { + ret = -ENOMEM; + goto error; + } + } else { + char *name; + + name = virXPathString(conn, "string(./owner)", ctxt); + if (!name) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("malformed owner element")); + goto error; + } + + perms->owner = name; + } } 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")); + if (virAsprintf(&perms->owner, "%d", getgid()) == -1) { + ret = -ENOMEM; goto error; } - perms->gid = (int)v; + } else { + if (virXPathLong(conn, "number(./group)", ctxt, &v) == 0) { + if (virAsprintf(&perms->group, "%ld", v) == -1) { + ret = -ENOMEM; + goto error; + } + } else { + char *name; + + name = virXPathString(conn, "string(./group)", ctxt); + if (!name) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("malformed group element")); + goto error; + } + + perms->group = name; + } } /* NB, we're ignoring missing labels here - they'll simply inherit */ @@ -815,10 +848,12 @@ virStoragePoolDefFormat(virConnectPtr conn, virBufferAddLit(&buf," <permissions>¥n"); virBufferVSprintf(&buf," <mode>0%o</mode>¥n", def->target.perms.mode); - virBufferVSprintf(&buf," <owner>%d</owner>¥n", - def->target.perms.uid); - virBufferVSprintf(&buf," <group>%d</group>¥n", - def->target.perms.gid); + if (def->target.perms.owner) + virBufferVSprintf(&buf," <owner>%s</owner>¥n", + def->target.perms.owner); + if (def->target.perms.group) + virBufferVSprintf(&buf," <group>%s</group>¥n", + def->target.perms.group); if (def->target.perms.label) virBufferVSprintf(&buf," <label>%s</label>¥n", @@ -1111,10 +1146,12 @@ virStorageVolTargetDefFormat(virConnectPtr conn, virBufferAddLit(buf," <permissions>¥n"); virBufferVSprintf(buf," <mode>0%o</mode>¥n", def->perms.mode); - virBufferVSprintf(buf," <owner>%d</owner>¥n", - def->perms.uid); - virBufferVSprintf(buf," <group>%d</group>¥n", - def->perms.gid); + if (def->perms.owner) + virBufferVSprintf(buf," <owner>%s</owner>¥n", + def->perms.owner); + if (def->perms.group) + virBufferVSprintf(buf," <group>%s</group>¥n", + def->perms.group); if (def->perms.label) diff --git a/src/storage_conf.h b/src/storage_conf.h index 4e35ccb..212e58c 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -36,8 +36,8 @@ typedef struct _virStoragePerms virStoragePerms; typedef virStoragePerms *virStoragePermsPtr; struct _virStoragePerms { int mode; - int uid; - int gid; + char *owner; + char *group; char *label; }; -- 1.6.0.6
On Thu, Apr 16, 2009 at 11:24:19PM +0900, Ryota Ozaki wrote:
Signed-off-by: Ryota Ozaki <ozaki.ryota@gmail.com>
From c441d5f29f1ed964e3c17dcac8614c0834eaba49 Mon Sep 17 00:00:00 2001 From: Ryota Ozaki <ozaki.ryota@gmail.com> Date: Thu, 16 Apr 2009 23:17:29 +0900 Subject: [PATCH] storage: allow alphabetical names in owner/group of permissions
--- docs/schemas/storagepool.rng | 10 ++++- docs/schemas/storagevol.rng | 10 ++++- src/Makefile.am | 1 + src/storage_backend.c | 8 +++- src/storage_backend_fs.c | 32 ++++++++++++++- src/storage_backend_logical.c | 33 ++++++++++++++- src/storage_conf.c | 91 +++++++++++++++++++++++++++++------------ src/storage_conf.h | 4 +- 8 files changed, 152 insertions(+), 37 deletions(-)
diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index e152e19..ed1e597 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -127,10 +127,16 @@ <ref name='uint'/> </element> <element name='owner'> - <ref name='uint'/> + <choice> + <ref name='uint'/> + <ref name='name'/> + </choice> </element> <element name='group'> - <ref name='uint'/> + <choice> + <ref name='uint'/> + <ref name='name'/> + </choice> </element> <optional> <element name='label'> diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index c7bd3a7..13e08b5 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -46,10 +46,16 @@ <ref name='uint'/> </element> <element name='owner'> - <ref name='uint'/> + <choice> + <ref name='uint'/> + <ref name='name'/> + </choice> </element> <element name='group'> - <ref name='uint'/> + <choice> + <ref name='uint'/> + <ref name='name'/> + </choice> </element> <optional> <element name='label'> diff --git a/src/Makefile.am b/src/Makefile.am index f176b46..84567aa 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -333,6 +333,7 @@ endif
# Needed to keep automake quiet about conditionals libvirt_driver_storage_la_SOURCES = +libvirt_driver_storage_la_LIBADD = libvirt_util.la if WITH_STORAGE_DIR if WITH_DRIVER_MODULES mod_LTLIBRARIES += libvirt_driver_storage.la
This isn't right. Instead you should list the symbols you need to link against in the src/libvirt_private.syms file.
@@ -420,25 +425,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")); + if (virAsprintf(&perms->owner, "%d", getuid()) == -1) { + ret = -ENOMEM; goto error; } - perms->uid = (int)v; + } else { + if (virXPathLong(conn, "number(./owner)", ctxt, &v) == 0) { + if (virAsprintf(&perms->owner, "%ld", v) == -1) { + ret = -ENOMEM; + goto error; + } + } else { + char *name; + + name = virXPathString(conn, "string(./owner)", ctxt); + if (!name) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("malformed owner element")); + goto error; + } + + perms->owner = name; + } }
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")); + if (virAsprintf(&perms->owner, "%d", getgid()) == -1) { + ret = -ENOMEM; goto error; } - perms->gid = (int)v; + } else { + if (virXPathLong(conn, "number(./group)", ctxt, &v) == 0) { + if (virAsprintf(&perms->group, "%ld", v) == -1) { + ret = -ENOMEM; + goto error; + } + } else { + char *name; + + name = virXPathString(conn, "string(./group)", ctxt); + if (!name) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("malformed group element")); + goto error; + } + + perms->group = name; + } }
/* NB, we're ignoring missing labels here - they'll simply inherit */ @@ -815,10 +848,12 @@ virStoragePoolDefFormat(virConnectPtr conn, virBufferAddLit(&buf," <permissions>¥n"); virBufferVSprintf(&buf," <mode>0%o</mode>¥n", def->target.perms.mode); - virBufferVSprintf(&buf," <owner>%d</owner>¥n", - def->target.perms.uid); - virBufferVSprintf(&buf," <group>%d</group>¥n", - def->target.perms.gid); + if (def->target.perms.owner) + virBufferVSprintf(&buf," <owner>%s</owner>¥n", + def->target.perms.owner); + if (def->target.perms.group) + virBufferVSprintf(&buf," <group>%s</group>¥n", + def->target.perms.group);
if (def->target.perms.label) virBufferVSprintf(&buf," <label>%s</label>¥n", @@ -1111,10 +1146,12 @@ virStorageVolTargetDefFormat(virConnectPtr conn, virBufferAddLit(buf," <permissions>¥n"); virBufferVSprintf(buf," <mode>0%o</mode>¥n", def->perms.mode); - virBufferVSprintf(buf," <owner>%d</owner>¥n", - def->perms.uid); - virBufferVSprintf(buf," <group>%d</group>¥n", - def->perms.gid); + if (def->perms.owner) + virBufferVSprintf(buf," <owner>%s</owner>¥n", + def->perms.owner); + if (def->perms.group) + virBufferVSprintf(buf," <group>%s</group>¥n", + def->perms.group);
if (def->perms.label)
Changing the XML format to put a name in place of the numeric ID is an incompatible schema change. If we want to include the name, then I'd suggest adding an attribute for it. When we generate XML, it would thus always output both the name and ID value. When parsing XML, it would accept only ID, or oinly name, or both the ID and name (and complain if the name doesn't match the ID). eg, when generating XML we'd always include <owner name='fred'>501</owner> when parsing XML we'd accept: <owner>501</owner> <owner name='fred'/> <owner name='fred'>501</owner>
diff --git a/src/storage_conf.h b/src/storage_conf.h index 4e35ccb..212e58c 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -36,8 +36,8 @@ typedef struct _virStoragePerms virStoragePerms; typedef virStoragePerms *virStoragePermsPtr; struct _virStoragePerms { int mode; - int uid; - int gid; + char *owner; + char *group; char *label; };
This shouldn't need to be change. the number values are the canonical representation of ownership, so that's what we shoul track internally in libvirt. The name <-> ID conversion can just be kept in the XML parser code. This should simplify the patch consierably, avoiding the need for any changes to the storage driver backends. 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 (2)
-
Daniel P. Berrange -
Ryota Ozaki