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

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(a)redhat.com&gt; > --- > 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.

> + </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.

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

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