On Thu, Apr 16, 2009 at 11:24:19PM +0900, Ryota Ozaki wrote:
> Signed-off-by: Ryota Ozaki <ozaki.ryota@xxxxxxxxx>
>
> >From c441d5f29f1ed964e3c17dcac8614c0834eaba49 Mon Sep 17 00:00:00 2001
> From: Ryota Ozaki <ozaki.ryota@xxxxxxxxx>
> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list