[libvirt] [PATCH v2 03/14] conf: Add/Allow parsing the auth in the disk source

[John Ferlan]

On 09/21/2017 08:38 AM, Peter Krempa wrote:
> On Fri, Sep 15, 2017 at 20:30:06 -0400, John Ferlan wrote:
>> Since the virStorageAuthDefPtr auth; is a member of _virStorageSource
>> it really should be allowed to be a subelement of the disk <source>
>> for the RBD and iSCSI prototcols. That way we can set up to allow
>> the <auth> element to be formatted within the disk source.
>>
>> For now just allow the format in the RNG and read it in domain_conf.
>>
>> Modify the qemuxml2argvtest to add a parse failure when there is an
>> <auth> as a child of <disk> *and* an <auth> as a child of <source>.
>>
>> The virschematest will read the new test files and validate from a
>> RNG viewpoint things are fine
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  docs/schemas/domaincommon.rng                      | 20 +++++++-
>>  src/conf/domain_conf.c                             | 53 ++++++++++++++++++++--
>>  ...v-disk-drive-network-iscsi-source-auth-both.xml | 36 +++++++++++++++
>>  ...l2argv-disk-drive-network-iscsi-source-auth.xml | 43 ++++++++++++++++++
>>  ...rgv-disk-drive-network-rbd-source-auth-both.xml | 45 ++++++++++++++++++
>>  ...xml2argv-disk-drive-network-rbd-source-auth.xml | 42 +++++++++++++++++
>>  tests/qemuxml2argvtest.c                           |  2 +
>>  7 files changed, 237 insertions(+), 4 deletions(-)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth-both.xml
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml
>>
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index c9a4f7a9a..139f1eea2 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -1578,11 +1578,29 @@
>>              <empty/>
>>            </element>
>>          </optional>
>> +        <optional>
>> +          <ref name="diskAuth"/>
>> +        </optional>
>>          <empty/>
>>        </interleave>
>>      </element>
>>    </define>
>>  
>> +  <define name="diskSourceNetworkProtocolISCSI">
>> +    <element name="source">
>> +      <attribute name="protocol">
>> +        <choice>
>> +          <value>iscsi</value>
>> +        </choice>
> 
> AFAIK Michal removed the <choice> here a few days ago.
> 

ah... probably after I had originally copied RBD...  I will remove it.

>> +      </attribute>
>> +      <attribute name="name"/>
>> +      <ref name="diskSourceNetworkHost"/>
>> +      <optional>
>> +        <ref name="diskAuth"/>
>> +      </optional>
>> +    </element>
>> +  </define>
>> +
>>    <define name="diskSourceNetworkProtocolHTTP">
>>      <element name="source">
>>        <attribute name="protocol">
> 
> 
> [...]
> 
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 8dca1357c..5c0218cdf 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
> 
> [...]
> 
>> @@ -8770,6 +8796,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
>>      char *serial = NULL;
>>      char *startupPolicy = NULL;
>>      virStorageAuthDefPtr authdef = NULL;
>> +    bool diskAuth = false;
> 
> I don't think you need this bool, since the variable above is non-null
> only in the correct cases and remains so until the end of the func when
> you steal the value.
> 
>>      char *tray = NULL;
>>      char *removable = NULL;
>>      char *logical_block_size = NULL;
> 
> [...]
> 
> 
> I think you will need to remember that the <auth> stuff was part of
> <disk> and not source, since we can't change that. More on that in
> review of the next patch.
> 

Right - I figured I'd give it a go with the absolute steal and replace
algorithm before going with the determine where <auth> was found and be
sure to output exactly as read in.

>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml
> 
> This file is not used. See below for a comment I had on the same case
> for RBD.
> 

Dang - thought I caught all those when I split things up.

>> new file mode 100644
>> index 000000000..af2d51fe9
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml
>> @@ -0,0 +1,43 @@
>> +<domain type='qemu'>
>> +  <name>QEMUGuest1</name>
>> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> 
> [...]

[...]

>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index c8c479cbd..d16b3b7b8 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -919,6 +919,8 @@ mymain(void)
>>      DO_TEST("disk-drive-network-iscsi-auth", NONE);
>>      DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-secrettype-invalid", NONE);
>>      DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-wrong-secrettype", NONE);
>> +    DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-source-auth-both", NONE);
>> +    DO_TEST_PARSE_ERROR("disk-drive-network-rbd-source-auth-both", NONE);
> 
> Do we really need both if the code paths in the parser are the same?
> 

No - they've been separate "historically", but I combine things and
clean up the testing portion a bit between this and the subsequent
patch.  Same for the encryption options.

It'll be a repost effort too...


thanks -

John