[libvirt] [PATCH 0/4] uid_t|gid_t fixes for 32 bit Linux
Hello, on 2012-11-23 I already reported a problem with running libvirt on 32 Bit Linuxes (libvirt 0.9.12 on Debian Squeeze in my case) on this ML: [BUG] storage.xml: owner|group=-1 → 2^32-1 on 32 Bit Due to multiple castings the initial -1 gets casted to (unsigned int)4294967295, which failes to be cased to an (signed int) on read-back. Thus the default storage pool is not launched: 2013-02-22 16:50:10.182+0000: 28550: info : libvirt version: 0.9.12 2013-02-22 16:50:10.182+0000: 28550: error : virStorageDefParsePerms:635 : XML error: malformed owner element This is caused by uid_t and gid_t being opaque types: their exact type ranges from s32 to u32 to u64. (Solaris until 2007, Linux, Windows64) The following patch series fixes several issues, which I found while investigating the storage pool startup problem: - mismatch between printf format and types - implicit casts of -1 to uid_t and gid_t - missing casts of uid_t and gid_t for printing - virXPathLong() failing on +(2^32-1) @Guido: for Debian only the 4th patch is relevant, which applies with some re-indention. Additional issues: - UID and GID 4294967295 happens to be the same as -1, so hopefully nobody uses it. Should this be documented somewhere? - Some regression test? Philipp Hahn (4): util: Fix printf format for uid_t|gid_t storage: Cast uid_t|gid_t to unsigned int storage: cast -1 for uid_t|gid_t storage: fix uid_t|gid_t handling on 32 bit Linux src/conf/storage_conf.c | 82 ++++++++++++++++++++++++++++++++--------- src/storage/storage_backend.c | 22 ++++++----- src/util/virutil.c | 10 ++--- 3 files changed, 81 insertions(+), 33 deletions(-) -- 1.7.10.4
The uid_t|gid_t values are explicitly casted to "unsigned long", but the printf() still used "%d", which is for signed values. Change the format to "%u". Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/util/virutil.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 7acda77..563f684 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2691,7 +2691,7 @@ virSetUIDGID(uid_t uid, gid_t gid) if (gid != (gid_t)-1) { if (setregid(gid, gid) < 0) { virReportSystemError(err = errno, - _("cannot change to '%d' group"), + _("cannot change to '%u' group"), (unsigned int) gid); goto error; } @@ -2722,7 +2722,7 @@ virSetUIDGID(uid_t uid, gid_t gid) } if (rc) { - virReportSystemError(err = rc, _("cannot getpwuid_r(%d)"), + virReportSystemError(err = rc, _("cannot getpwuid_r(%u)"), (unsigned int) uid); goto error; } @@ -2730,7 +2730,7 @@ virSetUIDGID(uid_t uid, gid_t gid) if (!pwd_result) { virReportError(VIR_ERR_INTERNAL_ERROR, _("getpwuid_r failed to retrieve data " - "for uid '%d'"), + "for uid '%u'"), (unsigned int) uid); err = EINVAL; goto error; @@ -2745,7 +2745,7 @@ virSetUIDGID(uid_t uid, gid_t gid) # endif if (setreuid(uid, uid) < 0) { virReportSystemError(err = errno, - _("cannot change to uid to '%d'"), + _("cannot change to uid to '%u'"), (unsigned int) uid); goto error; } -- 1.7.10.4
uid_t and gid_t are opaque types, ranging from s32 to u32 to u64. Explicitly cast the magic -1 to the appropriate type. Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/conf/storage_conf.c | 8 ++++---- src/storage/storage_backend.c | 16 ++++++++-------- src/util/virutil.c | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 7a39998..9134a22 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -674,8 +674,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, if (node == NULL) { /* Set default values if there is not <permissions> element */ perms->mode = defaultmode; - perms->uid = -1; - perms->gid = -1; + perms->uid = (uid_t) -1; + perms->gid = (gid_t) -1; perms->label = NULL; return 0; } @@ -700,7 +700,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } if (virXPathNode("./owner", ctxt) == NULL) { - perms->uid = -1; + perms->uid = (uid_t) -1; } else { if (virXPathLong("number(./owner)", ctxt, &v) < 0) { virReportError(VIR_ERR_XML_ERROR, @@ -711,7 +711,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } if (virXPathNode("./group", ctxt) == NULL) { - perms->gid = -1; + perms->gid = (gid_t) -1; } else { if (virXPathLong("number(./group)", ctxt, &v) < 0) { virReportError(VIR_ERR_XML_ERROR, diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 964ce29..ec2fa53 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -277,9 +277,9 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.path); goto cleanup; } - uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : -1; - gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : -1; - if (((uid != -1) || (gid != -1)) + uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : (uid_t) -1; + gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : (gid_t) -1; + if (((uid != (uid_t) -1) || (gid != (gid_t) -1)) && (fchown(fd, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), @@ -542,9 +542,9 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, if ((pool->def->type == VIR_STORAGE_POOL_NETFS) && (((getuid() == 0) - && (vol->target.perms.uid != -1) + && (vol->target.perms.uid != (uid_t) -1) && (vol->target.perms.uid != 0)) - || ((vol->target.perms.gid != -1) + || ((vol->target.perms.gid != (gid_t) -1) && (vol->target.perms.gid != getgid())))) { virCommandSetUID(cmd, vol->target.perms.uid); @@ -572,9 +572,9 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, } } - uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : -1; - gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : -1; - if (((uid != -1) || (gid != -1)) + uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : (uid_t) -1; + gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : (gid_t) -1; + if (((uid != (uid_t) -1) || (gid != (gid_t) -1)) && (chown(vol->target.path, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown %s to (%u, %u)"), diff --git a/src/util/virutil.c b/src/util/virutil.c index 563f684..4af2599 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1206,7 +1206,7 @@ parenterror: _("stat of '%s' failed"), path); goto childerror; } - if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + if ((st.st_gid != gid) && (chown(path, (uid_t) -1, gid) < 0)) { ret = -errno; virReportSystemError(errno, _("cannot chown '%s' to group %u"), -- 1.7.10.4
On 02/22/2013 09:41 AM, Philipp Hahn wrote:
uid_t and gid_t are opaque types, ranging from s32 to u32 to u64.
Or even s16 and u16 in older Unix.
Explicitly cast the magic -1 to the appropriate type.
Necessary in some cases, redundant in others; but overall good to be consistent.
Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/conf/storage_conf.c | 8 ++++---- src/storage/storage_backend.c | 16 ++++++++-------- src/util/virutil.c | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 7a39998..9134a22 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -674,8 +674,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, if (node == NULL) { /* Set default values if there is not <permissions> element */ perms->mode = defaultmode; - perms->uid = -1; - perms->gid = -1; + perms->uid = (uid_t) -1; + perms->gid = (gid_t) -1;
Redundant. C guarantees that -1 will promote correctly via assignment to any size integer, signed or unsigned.
perms->label = NULL; return 0; } @@ -700,7 +700,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, }
if (virXPathNode("./owner", ctxt) == NULL) { - perms->uid = -1; + perms->uid = (uid_t) -1;
Redundant.
} else { if (virXPathLong("number(./owner)", ctxt, &v) < 0) { virReportError(VIR_ERR_XML_ERROR, @@ -711,7 +711,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, }
if (virXPathNode("./group", ctxt) == NULL) { - perms->gid = -1; + perms->gid = (gid_t) -1;
Redundant.
} else { if (virXPathLong("number(./group)", ctxt, &v) < 0) { virReportError(VIR_ERR_XML_ERROR, diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 964ce29..ec2fa53 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -277,9 +277,9 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.path); goto cleanup; } - uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : -1; - gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : -1; - if (((uid != -1) || (gid != -1)) + uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : (uid_t) -1;
Necessary - ternary operator rules for integer promotion are not necessarily intuitive; and it is conceivable that mixing u16 with int can produce wrong results. Forcing both sides of the expression to be uid_t is indeed worthwhile.
+ gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : (gid_t) -1;
Ditto on being necessary.
+ if (((uid != (uid_t) -1) || (gid != (gid_t) -1))
And comparison also needs the cast - definitely necessary.
&& (fchown(fd, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), @@ -542,9 +542,9 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
if ((pool->def->type == VIR_STORAGE_POOL_NETFS) && (((getuid() == 0) - && (vol->target.perms.uid != -1) + && (vol->target.perms.uid != (uid_t) -1) && (vol->target.perms.uid != 0)) - || ((vol->target.perms.gid != -1) + || ((vol->target.perms.gid != (gid_t) -1)
Necessary.
&& (vol->target.perms.gid != getgid())))) {
virCommandSetUID(cmd, vol->target.perms.uid); @@ -572,9 +572,9 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, } }
- uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : -1; - gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : -1; - if (((uid != -1) || (gid != -1)) + uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : (uid_t) -1; + gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : (gid_t) -1; + if (((uid != (uid_t) -1) || (gid != (gid_t) -1))
Necessary.
&& (chown(vol->target.path, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown %s to (%u, %u)"), diff --git a/src/util/virutil.c b/src/util/virutil.c index 563f684..4af2599 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1206,7 +1206,7 @@ parenterror: _("stat of '%s' failed"), path); goto childerror; } - if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + if ((st.st_gid != gid) && (chown(path, (uid_t) -1, gid) < 0)) {
Redundant (integer promotion via prototyped function calls does the right thing). Since the patch is already pushed, I'm fine leaving the redundant parts in. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
uid_t and gid_t are opaque types, ranging from s32 to u32 to u64. Explicitly cast them to unsigned int for printing. Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/storage/storage_backend.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 019b09a..964ce29 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -283,7 +283,8 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, && (fchown(fd, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), - vol->target.path, uid, gid); + vol->target.path, (unsigned int) uid, + (unsigned int) gid); goto cleanup; } if (fchmod(fd, vol->target.perms.mode) < 0) { @@ -577,7 +578,8 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, && (chown(vol->target.path, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown %s to (%u, %u)"), - vol->target.path, uid, gid); + vol->target.path, (unsigned int) uid, + (unsigned int) gid); return -1; } if (chmod(vol->target.path, vol->target.perms.mode) < 0) { -- 1.7.10.4
On 02/22/2013 09:43 AM, Philipp Hahn wrote:
uid_t and gid_t are opaque types, ranging from s32 to u32 to u64.
Technically, I've never seen a platform with uid_t or gid_t being u64 - that's still theoretical at this time. However, since pid_t is 64-bit on mingw64, and since id_t is the union of [pug]id_t and therefore also a 64-bit type, I agree with this commit. Note that we have a compile-time assertion that on all platforms where libvirt is currently compiling, that sizeof(uid_t) <= sizeof(int) (likewise for gid_t). We'd have a LOT more code to clean up if we ever encounter a platform with u64 [ug]id_t. Still, since your commit has already been pushed, we can't touch up the commit message now. And ACK to your change. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
uid_t and gid_t are opaque types, ranging from s32 to u32 to u64. Unfortunately libvirt uses the value -1 to mean "current process", which on Linux32 gets converted to ALLONE := +(2^32-1) = 4294967295. Different libvirt versions used different formatting in the past, which break one or the other parsing function: virXPathLong(): (signed long)-1 != (double)ALLONE virXPathULong(): (unsigned long)-1 != (double)-1 Roll our own version of virXPathULong(), which also accepts -1, which is silently converted to ALLONE. For output: do the reverse and print -1 instead of ALLONE. Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/conf/storage_conf.c | 74 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 14 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 9134a22..b267f00 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -665,7 +665,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, const char *permxpath, int defaultmode) { char *mode; - long v; + double d; int ret = -1; xmlNodePtr relnode; xmlNodePtr node; @@ -699,26 +699,57 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, VIR_FREE(mode); } + /* uid_t and gid_t are *opaque* types: on Linux they're u32, on Windows64 + * they're u64, on Solaris they were s32 in the past. There may be others. + * + * Unfortunately libvirt uses the value -1 to mean "current process", which + * on Linux32 gets converted to ALLONE:=+(2^32-1). + * + * Different libvirt versions used different formatting in the past, which + * break one or the other parsing function: + * virXPathLong(): (signed long)-1 != (double)ALLONE + * virXPathULong(): (unsigned long)-1 != (double)-1 + */ if (virXPathNode("./owner", ctxt) == NULL) { perms->uid = (uid_t) -1; } else { - if (virXPathLong("number(./owner)", ctxt, &v) < 0) { + ret = virXPathNumber("number(./owner)", ctxt, &d); + if (ret == 0) { + if (d == (double) -1) { + perms->uid = (uid_t) -1; + } else { + perms->uid = (uid_t) (unsigned long) d; + if (perms->uid != d) { + ret = -2; + } + } + } + if (ret < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed owner element")); goto error; } - perms->uid = (int)v; } if (virXPathNode("./group", ctxt) == NULL) { perms->gid = (gid_t) -1; } else { - if (virXPathLong("number(./group)", ctxt, &v) < 0) { + ret = virXPathNumber("number(./group)", ctxt, &d); + if (ret == 0) { + if (d == (double) -1) { + perms->gid = (gid_t)-1; + } else { + perms->gid = (gid_t) (unsigned long) d; + if (perms->gid != d) { + ret = -2; + } + } + } + if (ret < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed group element")); goto error; } - perms->gid = (int)v; } /* NB, we're ignoring missing labels here - they'll simply inherit */ @@ -1060,10 +1091,18 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) { virBufferAddLit(&buf," <permissions>\n"); virBufferAsprintf(&buf," <mode>0%o</mode>\n", def->target.perms.mode); - virBufferAsprintf(&buf," <owner>%d</owner>\n", - (int) def->target.perms.uid); - virBufferAsprintf(&buf," <group>%d</group>\n", - (int) def->target.perms.gid); + if (def->target.perms.uid == (uid_t) -1) { + virBufferAddLit(&buf, " <owner>-1</owner>\n"); + } else { + virBufferAsprintf(&buf, " <owner>%u</owner>\n", + (unsigned int) def->target.perms.uid); + } + if (def->target.perms.gid == (gid_t) -1) { + virBufferAddLit(&buf, " <group>-1</group>\n"); + } else { + virBufferAsprintf(&buf, " <group>%u</group>\n", + (unsigned int) def->target.perms.gid); + } if (def->target.perms.label) virBufferAsprintf(&buf," <label>%s</label>\n", @@ -1313,11 +1352,18 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf," <permissions>\n"); virBufferAsprintf(buf," <mode>0%o</mode>\n", def->perms.mode); - virBufferAsprintf(buf," <owner>%u</owner>\n", - (unsigned int) def->perms.uid); - virBufferAsprintf(buf," <group>%u</group>\n", - (unsigned int) def->perms.gid); - + if (def->perms.uid == (uid_t) -1) { + virBufferAddLit(buf, " <owner>-1</owner>\n"); + } else { + virBufferAsprintf(buf, " <owner>%u</owner>\n", + (unsigned int) def->perms.uid); + } + if (def->perms.gid == (gid_t) -1) { + virBufferAddLit(buf, " <group>-1</group>\n"); + } else { + virBufferAsprintf(buf, " <group>%u</group>\n", + (unsigned int) def->perms.gid); + } if (def->perms.label) virBufferAsprintf(buf," <label>%s</label>\n", -- 1.7.10.4
On 22.02.2013 17:55, Philipp Hahn wrote:
uid_t and gid_t are opaque types, ranging from s32 to u32 to u64.
Unfortunately libvirt uses the value -1 to mean "current process", which on Linux32 gets converted to ALLONE := +(2^32-1) = 4294967295.
Different libvirt versions used different formatting in the past, which break one or the other parsing function: virXPathLong(): (signed long)-1 != (double)ALLONE virXPathULong(): (unsigned long)-1 != (double)-1
Roll our own version of virXPathULong(), which also accepts -1, which is silently converted to ALLONE.
For output: do the reverse and print -1 instead of ALLONE.
Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/conf/storage_conf.c | 74 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 14 deletions(-)
This one breaks storagevolxml2xmltest: TEST: storagevolxml2xmltest 1) Storage Vol XML-2-XML vol-file ... OK 2) Storage Vol XML-2-XML vol-file-backing ... OK 3) Storage Vol XML-2-XML vol-qcow2 ... OK 4) Storage Vol XML-2-XML vol-partition ... OK 5) Storage Vol XML-2-XML vol-logical ... OK 6) Storage Vol XML-2-XML vol-logical-backing ... OK 7) Storage Vol XML-2-XML vol-sheepdog ... Offset 283 Expect [4294967295</owner> <group>4294967295] Actual [-1</owner> <group>-1] ... FAILED However, the first 3 patches are correct. Even though it's freeze, they are pure bugfixes so I've pushed them. Michal
Hello, Am Montag 25 Februar 2013, 15:58:50 schrieb Michal Privoznik:
On 22.02.2013 17:55, Philipp Hahn wrote:
uid_t and gid_t are opaque types, ranging from s32 to u32 to u64. ... This one breaks storagevolxml2xmltest: TEST: storagevolxml2xmltest ... ... OK 7) Storage Vol XML-2-XML vol-sheepdog ... Offset 283 Expect [4294967295</owner> <group>4294967295] Actual [-1</owner> <group>-1] ... FAILED
The attached 1st patch would change the test case, but since I don't know anything about sheepdog, I can't say it that is really the correct change. @Sebastian: Is (uid_t)-1 = (u32)-1 special for Sheepdog or was the file just created by a previous virsh-dump, which outputs UINT_MAX instead of -1? There is one more interesting case in storagepoolxml2xmlout/pool-dir.xml, which should currently fail on 32 bit compiles, but does not, because pool- dumpxml only returns the original pool-xml with user/group=-1 (and updates capacities).
However, the first 3 patches are correct. Even though it's freeze, they are pure bugfixes so I've pushed them.
Thanks. On Monday 25 February 2013 19:46:50 Eric Blake wrote:
I like the standardized name INT_MAX better than the invented name ALLONE.
Better, but not correct: UINT_MAX is what is required here, because INT_MAX is the signed one with MSB=0.
+ double d;
Eww - why do we need to use a double?
struct _xmlXPathObject { xmlXPathObjectType type; xmlNodeSetPtr nodesetval; int boolval; double floatval; xmlChar *stringval; void *user; int index; void *user2; int index2; }; The xpath expression "Number(./owner)" must work for integer and floating point values, so a double is returned by "Number()". If you take a look at virXPathLongBase(), you'll see no such "double", because
Ask libxml2/libxml/xpath.h: the return value of "floatval" is immediately cased to a "long". The now used virXPathNumber() returns a "double".
I like the idea of outputting -1 rather than the unsigned all-ones value; which means you need to respin this patch to avoid testsuite failures where we change the test output.
See attached patch.
Also, anywhere that the parser doesn't accept -1, we need to fix that; accepting -1 as the only valid negative value and requiring all other values to be non-negative makes sense.
I had a look at the relax-ng schema: schemas/storagepool.rng accepts the -1, scheams/storagevol.rng does not. On the other hand the "-1" seems not to be documented on <http://libvirt.org/formatstorage.html> (btw: out of date, see 2. patch) The more I think about the -1 problem, the more I'm getting confused: in virStorageBackendFileSystemBuild() there is some code which reads back the actual owner and group, but my /etc/libvirt/storage/default.xml still contains the -1 / 4294967295, which also seems to be returned by "virsh pool-dumpxml default". Personally I would find the following semantics the least confusing: 1. on define, -1 stands for "the current gid / pid" for the future time, when the pool is started. 2. on create/start, -1 would be my gid/pid for qemu:///session and root:libvirt (or whatever) for qemu:///system . 3. on read back of an active pool, the current uid/gid are returned, so never -1. (the target exists, so the current values could be returned; a -1 is not very useful for the user here, IMHO.) 4. on read back of an passive pool, the original uid/gid (including -1) should be returned. (the target might not exist, so libvirt can only show the intended values) I would like to get that clarified before spending more time on a proper v2, since I now have to switch to a different task requiring my immediate attention.
- Some regression test?
Did you run 'make check' on your patch? We already have XML output that is affected by the change in output format, and can write the test so that we prove that multiple ways of formatting input all get canonicalized to the same output.
No, forgot to do that. Thank you for your review and comments. Much appreciated. Sincerely Philipp -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH be open. fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/ b/tests/storagevolxml2xmlout/vol-sheepdog.xml
Hi, Am 26.02.2013 um 13:06 schrieb Philipp Hahn <hahn@univention.de>:
@Sebastian: Is (uid_t)-1 = (u32)-1 special for Sheepdog or was the file just created by a previous virsh-dump, which outputs UINT_MAX instead of -1?
The sheepdog backend doesn't care about uid/gid. I probably created the file using virsh-dump. Best regards, Sebastian
On 02/22/2013 09:55 AM, Philipp Hahn wrote:
uid_t and gid_t are opaque types, ranging from s32 to u32 to u64.
Unfortunately libvirt uses the value -1 to mean "current process", which on Linux32 gets converted to ALLONE := +(2^32-1) = 4294967295.
I like the standardized name INT_MAX better than the invented name ALLONE.
Different libvirt versions used different formatting in the past, which break one or the other parsing function: virXPathLong(): (signed long)-1 != (double)ALLONE virXPathULong(): (unsigned long)-1 != (double)-1
Roll our own version of virXPathULong(), which also accepts -1, which is silently converted to ALLONE.
For output: do the reverse and print -1 instead of ALLONE.
Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/conf/storage_conf.c | 74 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 14 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 9134a22..b267f00 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -665,7 +665,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, const char *permxpath, int defaultmode) { char *mode; - long v; + double d;
Eww - why do we need to use a double? Just parse to a 32-bit int, then check for truncation after the fact (as I said elsewhere in the series, we already do a compile-time verify that uid_t is not larger than int; so the real problem is if uid_t is smaller than int).
int ret = -1; xmlNodePtr relnode; xmlNodePtr node; @@ -699,26 +699,57 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, VIR_FREE(mode); }
+ /* uid_t and gid_t are *opaque* types: on Linux they're u32, on Windows64 + * they're u64, on Solaris they were s32 in the past. There may be others.
Not accurate. mingw64 has s64 pid_t (not u64); and completely lacks uid_t and gid_t natively: $ printf '#include <sys/types.h>\nuid_t\n' | \ x86_64-w64-mingw32-gcc -E -|grep id_t typedef long long _pid_t; typedef _pid_t pid_t; uid_t When using gnulib (as libvirt does), gnulib provides [ug]id_t as 32-bit types even on mingw64. Furthermore, a lot of the problems stem from the fact that s16 or u16 uid_t/gid_t have been used in the past (u16 promotes to s32 without sign extension, so it does not compare equal to (s32)-1). I like the idea of outputting -1 rather than the unsigned all-ones value; which means you need to respin this patch to avoid testsuite failures where we change the test output. Also, anywhere that the parser doesn't accept -1, we need to fix that; accepting -1 as the only valid negative value and requiring all other values to be non-negative makes sense. Looking forward to v2; this patch is appropriate to get into the 1.0.3 release if we get it respun in time. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/22/2013 09:58 AM, Philipp Hahn wrote:
@Guido: for Debian only the 4th patch is relevant, which applies with some re-indention.
Unfortunately, the 4th patch is the one that needs rework.
Additional issues: - UID and GID 4294967295 happens to be the same as -1, so hopefully nobody uses it. Should this be documented somewhere?
Unless someone ever invents a system with 64-bit uid_t (to date, no one has - your claims about u64 to the contrary), treating 4294967295 as (uid_t)-1 is safe. POSIX already guaranteed that (uid_t)-1 cannot be used as a valid user id.
- Some regression test?
Did you run 'make check' on your patch? We already have XML output that is affected by the change in output format, and can write the test so that we prove that multiple ways of formatting input all get canonicalized to the same output. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Eric Blake -
Michal Privoznik -
Philipp Hahn -
Sebastian Wiedenroth