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 <jfer...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
