[libvirt] [PATCH] storage: unify permission formatting
Volume and pool formatting functions took different approaches to unspecified uids/gids. When unknown, it is always parsed as -1, but one of the functions formatted it as unsigned int (wrong) and one as int (better). Due to that, our two of our XML files from tests cannot be parsed on 32-bit machines. RNG schema needs to be modified as well, but because both storagepool.rng and storagevol.rng need same schema for permission element, save some space by moving it to storagecommon.rng. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/schemas/storagecommon.rng | 29 +++++++++++++++++++++++++ docs/schemas/storagepool.rng | 30 +------------------------- docs/schemas/storagevol.rng | 23 -------------------- src/conf/storage_conf.c | 9 ++++---- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 4 ++-- tests/storagevolxml2xmlout/vol-sheepdog.xml | 4 ++-- 6 files changed, 38 insertions(+), 61 deletions(-) diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 06b2f81..629505f 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -93,4 +93,33 @@ <notAllowed/> </define> + <define name='permissions'> + <optional> + <element name='permissions'> + <interleave> + <element name='mode'> + <ref name='octalMode'/> + </element> + <element name='owner'> + <choice> + <ref name='unsignedInt'/> + <value>-1</value> + </choice> + </element> + <element name='group'> + <choice> + <ref name='unsignedInt'/> + <value>-1</value> + </choice> + </element> + <optional> + <element name='label'> + <text/> + </element> + </optional> + </interleave> + </element> + </optional> + </define> + </grammar> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 0f05c5c..db6ff49 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -3,6 +3,7 @@ <grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> <include href='basictypes.rng'/> + <include href='storagecommon.rng'/> <start> <ref name='pool'/> </start> @@ -224,35 +225,6 @@ </interleave> </define> - <define name='permissions'> - <optional> - <element name='permissions'> - <interleave> - <element name='mode'> - <ref name='octalMode'/> - </element> - <element name='owner'> - <choice> - <ref name='unsignedInt'/> - <value>-1</value> - </choice> - </element> - <element name='group'> - <choice> - <ref name='unsignedInt'/> - <value>-1</value> - </choice> - </element> - <optional> - <element name='label'> - <text/> - </element> - </optional> - </interleave> - </element> - </optional> - </define> - <define name='target'> <element name='target'> <interleave> diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng index 1b2d4cc..7450547 100644 --- a/docs/schemas/storagevol.rng +++ b/docs/schemas/storagevol.rng @@ -59,29 +59,6 @@ </interleave> </define> - <define name='permissions'> - <optional> - <element name='permissions'> - <interleave> - <element name='mode'> - <ref name='octalMode'/> - </element> - <element name='owner'> - <ref name='unsignedInt'/> - </element> - <element name='group'> - <ref name='unsignedInt'/> - </element> - <optional> - <element name='label'> - <text/> - </element> - </optional> - </interleave> - </element> - </optional> - </define> - <define name='timestamps'> <optional> <element name='timestamps'> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 3987470..e1be064 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1203,7 +1203,6 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) (int) def->target.perms.uid); virBufferAsprintf(&buf, "<group>%d</group>\n", (int) def->target.perms.gid); - virBufferEscapeString(&buf, "<label>%s</label>\n", def->target.perms.label); @@ -1527,10 +1526,10 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, 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); + virBufferAsprintf(buf, "<owner>%d</owner>\n", + (int) def->perms->uid); + virBufferAsprintf(buf, "<group>%d</group>\n", + (int) def->perms->gid); virBufferEscapeString(buf, "<label>%s</label>\n", diff --git a/tests/storagevolxml2xmlout/vol-gluster-dir.xml b/tests/storagevolxml2xmlout/vol-gluster-dir.xml index f188ceb..538b31d 100644 --- a/tests/storagevolxml2xmlout/vol-gluster-dir.xml +++ b/tests/storagevolxml2xmlout/vol-gluster-dir.xml @@ -10,8 +10,8 @@ <format type='dir'/> <permissions> <mode>0600</mode> - <owner>4294967295</owner> - <group>4294967295</group> + <owner>-1</owner> + <group>-1</group> </permissions> </target> </volume> diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml index e08e36c..0a1f32c 100644 --- a/tests/storagevolxml2xmlout/vol-sheepdog.xml +++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml @@ -9,8 +9,8 @@ <format type='unknown'/> <permissions> <mode>0600</mode> - <owner>4294967295</owner> - <group>4294967295</group> + <owner>-1</owner> + <group>-1</group> </permissions> </target> </volume> -- 2.2.0
On 12/10/2014 06:00 AM, Martin Kletzander wrote:
Volume and pool formatting functions took different approaches to unspecified uids/gids. When unknown, it is always parsed as -1, but one of the functions formatted it as unsigned int (wrong) and one as int (better). Due to that, our two of our XML files from tests cannot be parsed on 32-bit machines.
In the past, when we've fixed things like this, we need to make sure that we can parse both old XML (that used 4294967295) and new XML (that uses -1). I think you'll need at least one xml2xml test where the input is old-style and output is new-style, to show that we make the conversion correctly (and thus, we don't break on upgrade from older style).
RNG schema needs to be modified as well, but because both storagepool.rng and storagevol.rng need same schema for permission element, save some space by moving it to storagecommon.rng.
That part makes sense, but I'm not quite ready to ACK without knowing for sure we are testing it thoroughly. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Dec 10, 2014 at 10:29:32AM -0700, Eric Blake wrote:
On 12/10/2014 06:00 AM, Martin Kletzander wrote:
Volume and pool formatting functions took different approaches to unspecified uids/gids. When unknown, it is always parsed as -1, but one of the functions formatted it as unsigned int (wrong) and one as int (better). Due to that, our two of our XML files from tests cannot be parsed on 32-bit machines.
In the past, when we've fixed things like this, we need to make sure that we can parse both old XML (that used 4294967295) and new XML (that uses -1). I think you'll need at least one xml2xml test where the input is old-style and output is new-style, to show that we make the conversion correctly (and thus, we don't break on upgrade from older style).
Oh, good point!
RNG schema needs to be modified as well, but because both storagepool.rng and storagevol.rng need same schema for permission element, save some space by moving it to storagecommon.rng.
That part makes sense, but I'm not quite ready to ACK without knowing for sure we are testing it thoroughly.
Definitely; version with tests added (and passing without additional modifications) is here: https://www.redhat.com/archives/libvir-list/2014-December/msg00611.html Martin
participants (2)
-
Eric Blake -
Martin Kletzander