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@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