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.
> + </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/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml
>
> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml
> new file mode 100644
> index 000000000..62a781cd3
> --- /dev/null
> +++
> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml
> @@ -0,0 +1,45 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219136</memory>
> + <currentMemory unit='KiB'>219136</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='i686' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-i686</emulator>
> + <disk type='block' device='disk'>
> + <driver name='qemu' type='raw'/>
> + <source dev='/dev/HostVG/QEMUGuest1'/>
> + <target dev='hda' bus='ide'/>
> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
This disk is not needed.
> + </disk>
> + <disk type='network' device='disk'>
> + <driver name='qemu' type='raw'/>
> + <auth username='myname'>
> + <secret type='ceph' usage='mycluster_myname'/>
> + </auth>
> + <source protocol='rbd' name='pool/image'>
> + <host name='mon1.example.org' port='6321'/>
> + <host name='mon2.example.org' port='6322'/>
> + <host name='mon3.example.org' port='6322'/>
> + <auth username='myname'>
> + <secret type='ceph' usage='mycluster_myname'/>
> + </auth>
> + </source>
> + <target dev='vda' bus='virtio'/>
> + </disk>
> + <controller type='usb' index='0'/>
> + <controller type='ide' index='0'/>
> + <controller type='pci' index='0' model='pci-root'/>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='none'/>
> + </devices>
> +</domain>
> diff --git
> a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml
> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml
This file is added but not used in any test. Also when you are going to
make it used later, can't you just add another disk to an existing test
case? We have a few of cases which already test RBD auth. There's no
need to add so much of the bloat just to test the new syntax.
> new file mode 100644
> index 000000000..d25e4148b
> --- /dev/null
> +++
> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml
> @@ -0,0 +1,42 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219136</memory>
> + <currentMemory unit='KiB'>219136</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='i686' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-i686</emulator>
> + <disk type='block' device='disk'>
> + <driver name='qemu' type='raw'/>
> + <source dev='/dev/HostVG/QEMUGuest1'/>
> + <target dev='hda' bus='ide'/>
> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> + </disk>
Lots of unnecessary stuff ...
> + <disk type='network' device='disk'>
> + <driver name='qemu' type='raw'/>
> + <source protocol='rbd' name='pool/image'>
> + <host name='mon1.example.org' port='6321'/>
> + <host name='mon2.example.org' port='6322'/>
> + <host name='mon3.example.org' port='6322'/>
> + <auth username='myname'>
> + <secret type='ceph' usage='mycluster_myname'/>
> + </auth>
> + </source>
> + <target dev='vda' bus='virtio'/>
> + </disk>
[...]
> 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?
ACK with the test file stuff sorted out and the fix to the schema
change.
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
