[libvirt] [PATCH v3 2/4] storage: conf: Don't set any default <mode> in the XML

[Cole Robinson]

On 05/24/2015 07:37 AM, John Ferlan wrote:
> 
> 
> On 05/21/2015 04:03 PM, Cole Robinson wrote:
>> The XML parser sets a default <mode> if none is explicitly passed in.
>> This is then used at pool/vol creation time, and unconditionally reported
>> in the XML.
>>
>> The problem with this approach is that it's impossible for other code
>> to determine if the user explicitly requested a storage mode. There
>> are some cases where we want to make this distinction, but we currently
>> can't.
>>
>> Handle <mode> parsing like we handle <owner>/<group>: if no value is
>> passed in, set it to -1, and adjust the internal consumers to handle
>> it.
>> ---
>> v3:
>>     Drop needless test churn
>>     Add docs about default <mode>
>>
>>  docs/formatstorage.html.in                         |  4 +++
>>  docs/schemas/storagecommon.rng                     |  8 +++--
>>  src/conf/storage_conf.c                            | 34 +++++++++-------------
>>  src/storage/storage_backend.c                      | 20 +++++++++----
>>  src/storage/storage_backend.h                      |  3 ++
>>  src/storage/storage_backend_fs.c                   |  9 ++++--
>>  src/storage/storage_backend_logical.c              |  4 ++-
>>  tests/storagepoolxml2xmlout/pool-netfs-gluster.xml |  1 -
>>  tests/storagevolxml2xmlout/vol-gluster-dir.xml     |  1 -
>>  tests/storagevolxml2xmlout/vol-sheepdog.xml        |  1 -
>>  10 files changed, 51 insertions(+), 34 deletions(-)
>>
>> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
>> index f07bb5d..ccde978 100644
>> --- a/docs/formatstorage.html.in
>> +++ b/docs/formatstorage.html.in
>> @@ -406,6 +406,8 @@
>>          namespace. It provides information about the permissions to use for the
>>          final directory when the pool is built. The
>>          <code>mode</code> element contains the octal permission set.
>> +        If <code>mode</code> is unset when creating a directory,
>> +        the value 0755 is used.
> 
> Consider "The mode defaults to 0755 when not provided."
> 
> The verbiage "is unset" reads strangly
> 

Thanks, fixed.

>>          The <code>owner</code> element contains the numeric user ID.
>>          The <code>group</code> element contains the numeric group ID.
>>          If <code>owner</code> or <code>group</code> aren't specified when
>> @@ -595,6 +597,8 @@
>>          files. For pools where the volumes are device nodes, the hotplug
>>          scripts determine permissions. It contains 4 child elements. The
>>          <code>mode</code> element contains the octal permission set.
>> +        If <code>mode</code> is unset when creating a supported volume,
>> +        the value 0600 is used.
> 
> ditto but 0600
> 
> Also, wherever you "point" the "<backing store elements> <permissions>
> to should be whichever default is correct for the backing store. that is
> from patch 1, you indicated that the elements are the same as one of
> these two elements, so to be technically correct be sure to redirect to
> either the 0755 or 0600 for which the backing store element would
> default to if not provided.
> 

It's already saying 'see volume docs' essentially, so this should be fine.

Pushed with the change mentioned above.

Thanks,
Cole