[libvirt] [PATCH 00/14] network schema refactor and addition of cookies for http disks
Peter Krempa (14): docs: schemas: Remove <interleave> from file/block/dir/volume disks docs: schemas: Extract disk source host specification docs: schemas: Move the interleave definition into network disk source docs: schemas: Extract RBD-specific data docs: schemas: Extract HTTP disk source specification docs: schemas: Split out simple network protocols docs: schemas: Split up definitions for NBD and gluster conf: Extract formatting of network disk source into separate function tests: genericxml2xml: Add test case for HTTP based disk util: string: Introduce virStringHasChars conf: Add support for cookies for HTTP based disks qemu: command: Use switch to formatting additional network disk params qemu: capabilities: Add capability for the new curl driver options qemu: command: Add support for HTTP cookies docs/formatdomain.html.in | 9 + docs/schemas/domaincommon.rng | 316 +++++++++++++-------- src/conf/domain_conf.c | 205 ++++++++++--- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 48 +++- src/util/virstoragefile.c | 124 ++++++++ src/util/virstoragefile.h | 14 + src/util/virstring.c | 23 +- src/util/virstring.h | 2 + .../generic-disk-network-http.xml | 48 ++++ tests/genericxml2xmltest.c | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + .../qemuxml2argv-disk-drive-network-http.args | 32 +++ .../qemuxml2argv-disk-drive-network-http.xml | 48 ++++ tests/qemuxml2argvtest.c | 1 + 16 files changed, 701 insertions(+), 174 deletions(-) create mode 100644 tests/genericxml2xmlindata/generic-disk-network-http.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.xml -- 2.12.2
They don't contain any elements to interleave. --- docs/schemas/domaincommon.rng | 136 ++++++++++++++++++++---------------------- 1 file changed, 64 insertions(+), 72 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index eb4b0f743..79ea56960 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1430,65 +1430,59 @@ <value>file</value> </attribute> </optional> - <interleave> - <optional> - <element name="source"> - <optional> - <attribute name="file"> - <ref name="absFilePath"/> - </attribute> - </optional> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <zeroOrMore> - <ref name='devSeclabel'/> - </zeroOrMore> - </element> - </optional> - </interleave> + <optional> + <element name="source"> + <optional> + <attribute name="file"> + <ref name="absFilePath"/> + </attribute> + </optional> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <zeroOrMore> + <ref name='devSeclabel'/> + </zeroOrMore> + </element> + </optional> </define> <define name="diskSourceBlock"> <attribute name="type"> <value>block</value> </attribute> - <interleave> - <optional> - <element name="source"> - <optional> - <attribute name="dev"> - <ref name="absFilePath"/> - </attribute> - </optional> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <zeroOrMore> - <ref name='devSeclabel'/> - </zeroOrMore> - </element> - </optional> - </interleave> + <optional> + <element name="source"> + <optional> + <attribute name="dev"> + <ref name="absFilePath"/> + </attribute> + </optional> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <zeroOrMore> + <ref name='devSeclabel'/> + </zeroOrMore> + </element> + </optional> </define> <define name="diskSourceDir"> <attribute name="type"> <value>dir</value> </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="dir"> - <ref name="absFilePath"/> - </attribute> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <empty/> - </element> - </optional> - </interleave> + <optional> + <element name="source"> + <attribute name="dir"> + <ref name="absFilePath"/> + </attribute> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <empty/> + </element> + </optional> </define> <define name="diskSourceNetwork"> @@ -1574,32 +1568,30 @@ <attribute name="type"> <value>volume</value> </attribute> - <interleave> - <optional> - <element name="source"> - <attribute name="pool"> - <ref name="genericName"/> - </attribute> - <attribute name="volume"> - <ref name="volName"/> + <optional> + <element name="source"> + <attribute name="pool"> + <ref name="genericName"/> + </attribute> + <attribute name="volume"> + <ref name="volName"/> + </attribute> + <optional> + <attribute name="mode"> + <choice> + <value>host</value> + <value>direct</value> + </choice> </attribute> - <optional> - <attribute name="mode"> - <choice> - <value>host</value> - <value>direct</value> - </choice> - </attribute> - </optional> - <optional> - <ref name="storageStartupPolicy"/> - </optional> - <zeroOrMore> - <ref name='devSeclabel'/> - </zeroOrMore> - </element> - </optional> - </interleave> + </optional> + <optional> + <ref name="storageStartupPolicy"/> + </optional> + <zeroOrMore> + <ref name='devSeclabel'/> + </zeroOrMore> + </element> + </optional> </define> <define name="diskTarget"> -- 2.12.2
On Wed, Apr 26, 2017 at 19:52:31 +0200, Peter Krempa wrote:
They don't contain any elements to interleave. --- docs/schemas/domaincommon.rng | 136 ++++++++++++++++++++---------------------- 1 file changed, 64 insertions(+), 72 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
'diskSourceNetwork' schema define was rather big and it would be hard to simplify it. Split out the host portion subelement into a separate define. --- docs/schemas/domaincommon.rng | 70 +++++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 33 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 79ea56960..5d17809b3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1485,6 +1485,42 @@ </optional> </define> + <define name="diskSourceNetworkHost"> + <element name="host"> + <choice> + <group> + <optional> + <attribute name="transport"> + <choice> + <value>tcp</value> + <value>rdma</value> + </choice> + </attribute> + </optional> + <attribute name="name"> + <choice> + <ref name="dnsName"/> + <ref name="ipAddr"/> + </choice> + </attribute> + <optional> + <attribute name="port"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </group> + <group> + <attribute name="transport"> + <value>unix</value> + </attribute> + <attribute name="socket"> + <ref name="absFilePath"/> + </attribute> + </group> + </choice> + </element> + </define> + <define name="diskSourceNetwork"> <attribute name="type"> <value>network</value> @@ -1509,39 +1545,7 @@ <attribute name="name"/> </optional> <zeroOrMore> - <element name="host"> - <choice> - <group> - <optional> - <attribute name="transport"> - <choice> - <value>tcp</value> - <value>rdma</value> - </choice> - </attribute> - </optional> - <attribute name="name"> - <choice> - <ref name="dnsName"/> - <ref name="ipAddr"/> - </choice> - </attribute> - <optional> - <attribute name="port"> - <ref name="unsignedInt"/> - </attribute> - </optional> - </group> - <group> - <attribute name="transport"> - <value>unix</value> - </attribute> - <attribute name="socket"> - <ref name="absFilePath"/> - </attribute> - </group> - </choice> - </element> + <ref name="diskSourceNetworkHost"/> </zeroOrMore> <optional> <element name="snapshot"> -- 2.12.2
On Wed, Apr 26, 2017 at 19:52:32 +0200, Peter Krempa wrote:
'diskSourceNetwork' schema define was rather big and it would be hard to simplify it. Split out the host portion subelement into a separate define. --- docs/schemas/domaincommon.rng | 70 +++++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 33 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Move it to the place where actually interleaving elements can be placed. --- docs/schemas/domaincommon.rng | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 5d17809b3..eca479594 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1525,8 +1525,8 @@ <attribute name="type"> <value>network</value> </attribute> - <interleave> - <element name="source"> + <element name="source"> + <interleave> <attribute name="protocol"> <choice> <value>nbd</value> @@ -1564,8 +1564,8 @@ </element> </optional> <empty/> - </element> - </interleave> + </interleave> + </element> </define> <define name="diskSourceVolume"> -- 2.12.2
On Wed, Apr 26, 2017 at 19:52:33 +0200, Peter Krempa wrote:
Move it to the place where actually interleaving elements can be placed. --- docs/schemas/domaincommon.rng | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 5d17809b3..eca479594 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1525,8 +1525,8 @@ <attribute name="type"> <value>network</value> </attribute> - <interleave> - <element name="source"> + <element name="source"> + <interleave> <attribute name="protocol"> <choice> <value>nbd</value> @@ -1564,8 +1564,8 @@ </element> </optional> <empty/> - </element> - </interleave> + </interleave> + </element> </define>
<define name="diskSourceVolume">
You could have left the attributes outside <interleave>, but it doesn't matter much. Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
RBD driver supports specifying a snapshot image name or config file. Create a define for RBD and move the specifics there. --- docs/schemas/domaincommon.rng | 50 ++++++++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index eca479594..05efea7f2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1521,24 +1521,12 @@ </element> </define> - <define name="diskSourceNetwork"> - <attribute name="type"> - <value>network</value> - </attribute> + <define name="diskSourceNetworkProtocolRBD"> <element name="source"> <interleave> <attribute name="protocol"> <choice> - <value>nbd</value> <value>rbd</value> - <value>sheepdog</value> - <value>gluster</value> - <value>iscsi</value> - <value>http</value> - <value>https</value> - <value>ftp</value> - <value>ftps</value> - <value>tftp</value> </choice> </attribute> <optional> @@ -1568,6 +1556,42 @@ </element> </define> + <define name="diskSourceNetworkProtocolGeneric"> + <element name="source"> + <interleave> + <attribute name="protocol"> + <choice> + <value>nbd</value> + <value>sheepdog</value> + <value>gluster</value> + <value>iscsi</value> + <value>http</value> + <value>https</value> + <value>ftp</value> + <value>ftps</value> + <value>tftp</value> + </choice> + </attribute> + <optional> + <attribute name="name"/> + </optional> + <zeroOrMore> + <ref name="diskSourceNetworkHost"/> + </zeroOrMore> + </interleave> + </element> + </define> + + <define name="diskSourceNetwork"> + <attribute name="type"> + <value>network</value> + </attribute> + <choice> + <ref name="diskSourceNetworkProtocolGeneric"/> + <ref name="diskSourceNetworkProtocolRBD"/> + </choice> + </define> + <define name="diskSourceVolume"> <attribute name="type"> <value>volume</value> -- 2.12.2
On Wed, Apr 26, 2017 at 19:52:34 +0200, Peter Krempa wrote:
RBD driver supports specifying a snapshot image name or config file. Create a define for RBD and move the specifics there. --- docs/schemas/domaincommon.rng | 50 ++++++++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 13 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index eca479594..05efea7f2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng ... @@ -1568,6 +1556,42 @@ </element> </define>
+ <define name="diskSourceNetworkProtocolGeneric"> + <element name="source"> + <interleave> + <attribute name="protocol"> + <choice> + <value>nbd</value> + <value>sheepdog</value> + <value>gluster</value> + <value>iscsi</value> + <value>http</value> + <value>https</value> + <value>ftp</value> + <value>ftps</value> + <value>tftp</value> + </choice> + </attribute> + <optional> + <attribute name="name"/> + </optional> + <zeroOrMore> + <ref name="diskSourceNetworkHost"/> + </zeroOrMore> + </interleave> + </element> + </define>
The <interleave> is redundant here as attributes are implicitly interleaved and there's only a single <host> element in addition to them.
+ + <define name="diskSourceNetwork"> + <attribute name="type"> + <value>network</value> + </attribute> + <choice> + <ref name="diskSourceNetworkProtocolGeneric"/> + <ref name="diskSourceNetworkProtocolRBD"/> + </choice> + </define> +
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
On Thu, Apr 27, 2017 at 16:48:17 +0200, Jiri Denemark wrote:
On Wed, Apr 26, 2017 at 19:52:34 +0200, Peter Krempa wrote:
RBD driver supports specifying a snapshot image name or config file. Create a define for RBD and move the specifics there. --- docs/schemas/domaincommon.rng | 50 ++++++++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 13 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index eca479594..05efea7f2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng ... @@ -1568,6 +1556,42 @@ </element> </define>
+ <define name="diskSourceNetworkProtocolGeneric"> + <element name="source"> + <interleave> + <attribute name="protocol"> + <choice> + <value>nbd</value> + <value>sheepdog</value> + <value>gluster</value> + <value>iscsi</value> + <value>http</value> + <value>https</value> + <value>ftp</value> + <value>ftps</value> + <value>tftp</value> + </choice> + </attribute> + <optional> + <attribute name="name"/> + </optional> + <zeroOrMore> + <ref name="diskSourceNetworkHost"/> + </zeroOrMore> + </interleave> + </element> + </define>
The <interleave> is redundant here as attributes are implicitly interleaved and there's only a single <host> element in addition to them.
This is a diff quirk. I did not touch this part, I just renamed it.
Make the schema more strict for HTTP disks requiring a name and mandating exactly one source host. ftp/tftp entries were not moved here, since http transport also will support cookies and other options, which will be added later. --- docs/schemas/domaincommon.rng | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 05efea7f2..e3dc34e08 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1556,6 +1556,19 @@ </element> </define> + <define name="diskSourceNetworkProtocolHTTP"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>http</value> + <value>https</value> + </choice> + </attribute> + <attribute name="name"/> + <ref name="diskSourceNetworkHost"/> + </element> + </define> + <define name="diskSourceNetworkProtocolGeneric"> <element name="source"> <interleave> @@ -1565,8 +1578,6 @@ <value>sheepdog</value> <value>gluster</value> <value>iscsi</value> - <value>http</value> - <value>https</value> <value>ftp</value> <value>ftps</value> <value>tftp</value> @@ -1589,6 +1600,7 @@ <choice> <ref name="diskSourceNetworkProtocolGeneric"/> <ref name="diskSourceNetworkProtocolRBD"/> + <ref name="diskSourceNetworkProtocolHTTP"/> </choice> </define> -- 2.12.2
On Wed, Apr 26, 2017 at 19:52:35 +0200, Peter Krempa wrote:
Make the schema more strict for HTTP disks requiring a name and mandating exactly one source host.
ftp/tftp entries were not moved here, since http transport also will support cookies and other options, which will be added later. --- docs/schemas/domaincommon.rng | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 05efea7f2..e3dc34e08 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1556,6 +1556,19 @@ </element> </define>
+ <define name="diskSourceNetworkProtocolHTTP"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>http</value> + <value>https</value> + </choice> + </attribute> + <attribute name="name"/>
The @name attribute was originally optional. Was it optional because some protocols don't need it by http(s) needs the name? Or should it be optional even for http(s)?
+ <ref name="diskSourceNetworkHost"/> + </element> + </define>
Jirka
On Thu, Apr 27, 2017 at 16:56:13 +0200, Jiri Denemark wrote:
On Wed, Apr 26, 2017 at 19:52:35 +0200, Peter Krempa wrote:
Make the schema more strict for HTTP disks requiring a name and mandating exactly one source host.
ftp/tftp entries were not moved here, since http transport also will support cookies and other options, which will be added later. --- docs/schemas/domaincommon.rng | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 05efea7f2..e3dc34e08 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1556,6 +1556,19 @@ </element> </define>
+ <define name="diskSourceNetworkProtocolHTTP"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>http</value> + <value>https</value> + </choice> + </attribute> + <attribute name="name"/>
The @name attribute was originally optional. Was it optional because some protocols don't need it by http(s) needs the name? Or should it be optional even for http(s)?
@name is optional only for NBD
On Thu, Apr 27, 2017 at 17:15:59 +0200, Peter Krempa wrote:
On Thu, Apr 27, 2017 at 16:56:13 +0200, Jiri Denemark wrote:
On Wed, Apr 26, 2017 at 19:52:35 +0200, Peter Krempa wrote:
Make the schema more strict for HTTP disks requiring a name and mandating exactly one source host.
ftp/tftp entries were not moved here, since http transport also will support cookies and other options, which will be added later. --- docs/schemas/domaincommon.rng | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 05efea7f2..e3dc34e08 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1556,6 +1556,19 @@ </element> </define>
+ <define name="diskSourceNetworkProtocolHTTP"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>http</value> + <value>https</value> + </choice> + </attribute> + <attribute name="name"/>
The @name attribute was originally optional. Was it optional because some protocols don't need it by http(s) needs the name? Or should it be optional even for http(s)?
@name is optional only for NBD
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
ftp/tftp/sheepdog have a mandatory filename and support only one host. There are no additional options for them. --- docs/schemas/domaincommon.rng | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index e3dc34e08..d5e6bdb6b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1569,18 +1569,29 @@ </element> </define> + <define name="diskSourceNetworkProtocolSimple"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>sheepdog</value> + <value>iscsi</value> + <value>ftp</value> + <value>ftps</value> + <value>tftp</value> + </choice> + </attribute> + <attribute name="name"/> + <ref name="diskSourceNetworkHost"/> + </element> + </define> + <define name="diskSourceNetworkProtocolGeneric"> <element name="source"> <interleave> <attribute name="protocol"> <choice> <value>nbd</value> - <value>sheepdog</value> <value>gluster</value> - <value>iscsi</value> - <value>ftp</value> - <value>ftps</value> - <value>tftp</value> </choice> </attribute> <optional> @@ -1601,6 +1612,7 @@ <ref name="diskSourceNetworkProtocolGeneric"/> <ref name="diskSourceNetworkProtocolRBD"/> <ref name="diskSourceNetworkProtocolHTTP"/> + <ref name="diskSourceNetworkProtocolSimple"/> </choice> </define> -- 2.12.2
On Wed, Apr 26, 2017 at 19:52:36 +0200, Peter Krempa wrote:
ftp/tftp/sheepdog have a mandatory filename and support only one host. There are no additional options for them. --- docs/schemas/domaincommon.rng | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
NBD does not mandate a "filename". Gluster can have more servers. Split them so that we can tighten the schema. --- docs/schemas/domaincommon.rng | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index d5e6bdb6b..0c51f5151 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1585,31 +1585,42 @@ </element> </define> - <define name="diskSourceNetworkProtocolGeneric"> + <define name="diskSourceNetworkProtocolNBD"> <element name="source"> - <interleave> - <attribute name="protocol"> - <choice> - <value>nbd</value> - <value>gluster</value> - </choice> - </attribute> - <optional> - <attribute name="name"/> - </optional> - <zeroOrMore> - <ref name="diskSourceNetworkHost"/> - </zeroOrMore> - </interleave> + <attribute name="protocol"> + <choice> + <value>nbd</value> + </choice> + </attribute> + <optional> + <attribute name="name"/> + </optional> + <ref name="diskSourceNetworkHost"/> + </element> + </define> + + <define name="diskSourceNetworkProtocolGluster"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>gluster</value> + </choice> + </attribute> + <attribute name="name"/> + <oneOrMore> + <ref name="diskSourceNetworkHost"/> + </oneOrMore> </element> </define> + <define name="diskSourceNetwork"> <attribute name="type"> <value>network</value> </attribute> <choice> - <ref name="diskSourceNetworkProtocolGeneric"/> + <ref name="diskSourceNetworkProtocolNBD"/> + <ref name="diskSourceNetworkProtocolGluster"/> <ref name="diskSourceNetworkProtocolRBD"/> <ref name="diskSourceNetworkProtocolHTTP"/> <ref name="diskSourceNetworkProtocolSimple"/> -- 2.12.2
On Wed, Apr 26, 2017 at 19:52:37 +0200, Peter Krempa wrote:
NBD does not mandate a "filename". Gluster can have more servers. Split them so that we can tighten the schema. --- docs/schemas/domaincommon.rng | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index d5e6bdb6b..0c51f5151 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1585,31 +1585,42 @@ </element> </define>
- <define name="diskSourceNetworkProtocolGeneric"> + <define name="diskSourceNetworkProtocolNBD"> <element name="source"> - <interleave> - <attribute name="protocol"> - <choice> - <value>nbd</value> - <value>gluster</value> - </choice> - </attribute> - <optional> - <attribute name="name"/> - </optional> - <zeroOrMore> - <ref name="diskSourceNetworkHost"/> - </zeroOrMore> - </interleave> + <attribute name="protocol"> + <choice> + <value>nbd</value> + </choice> + </attribute> + <optional> + <attribute name="name"/> + </optional> + <ref name="diskSourceNetworkHost"/> + </element> + </define> + + <define name="diskSourceNetworkProtocolGluster"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>gluster</value> + </choice> + </attribute> + <attribute name="name"/> + <oneOrMore> + <ref name="diskSourceNetworkHost"/> + </oneOrMore> </element> </define>
+
This looks like an unintended new line.
<define name="diskSourceNetwork"> <attribute name="type"> <value>network</value> </attribute> <choice> - <ref name="diskSourceNetworkProtocolGeneric"/> + <ref name="diskSourceNetworkProtocolNBD"/> + <ref name="diskSourceNetworkProtocolGluster"/> <ref name="diskSourceNetworkProtocolRBD"/> <ref name="diskSourceNetworkProtocolHTTP"/> <ref name="diskSourceNetworkProtocolSimple"/>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
--- src/conf/domain_conf.c | 99 ++++++++++++++++++++++++++------------------------ 1 file changed, 52 insertions(+), 47 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 61006dea7..5a736c853 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20767,6 +20767,56 @@ virDomainSourceDefFormatSeclabel(virBufferPtr buf, virDomainDiskSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags, false); } + +static int +virDomainDiskSourceFormatNetwork(virBufferPtr buf, + virStorageSourcePtr src) +{ + size_t n; + char *path = NULL; + + virBufferAsprintf(buf, "<source protocol='%s'", + virStorageNetProtocolTypeToString(src->protocol)); + + if (src->volume) { + if (virAsprintf(&path, "%s%s", src->volume, src->path) < 0) + return -1; + } + + virBufferEscapeString(buf, " name='%s'", path ? path : src->path); + + VIR_FREE(path); + + if (src->nhosts == 0 && !src->snapshot && !src->configFile) { + virBufferAddLit(buf, "/>\n"); + } else { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + + for (n = 0; n < src->nhosts; n++) { + virBufferAddLit(buf, "<host"); + virBufferEscapeString(buf, " name='%s'", src->hosts[n].name); + virBufferEscapeString(buf, " port='%s'", src->hosts[n].port); + + if (src->hosts[n].transport) + virBufferAsprintf(buf, " transport='%s'", + virStorageNetHostTransportTypeToString(src->hosts[n].transport)); + + virBufferEscapeString(buf, " socket='%s'", src->hosts[n].socket); + virBufferAddLit(buf, "/>\n"); + } + + virBufferEscapeString(buf, "<snapshot name='%s'/>\n", src->snapshot); + virBufferEscapeString(buf, "<config file='%s'/>\n", src->configFile); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</source>\n"); + } + + return 0; +} + + static int virDomainDiskSourceFormatInternal(virBufferPtr buf, virStorageSourcePtr src, @@ -20774,8 +20824,6 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, unsigned int flags, bool skipSeclabels) { - size_t n; - char *path = NULL; const char *startupPolicy = NULL; if (policy) @@ -20811,51 +20859,8 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, break; case VIR_STORAGE_TYPE_NETWORK: - virBufferAsprintf(buf, "<source protocol='%s'", - virStorageNetProtocolTypeToString(src->protocol)); - - - if (src->volume) { - if (virAsprintf(&path, "%s%s", src->volume, src->path) < 0) - return -1; - } - - virBufferEscapeString(buf, " name='%s'", path ? path : src->path); - - VIR_FREE(path); - - if (src->nhosts == 0 && !src->snapshot && !src->configFile) { - virBufferAddLit(buf, "/>\n"); - } else { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - - for (n = 0; n < src->nhosts; n++) { - virBufferAddLit(buf, "<host"); - virBufferEscapeString(buf, " name='%s'", - src->hosts[n].name); - virBufferEscapeString(buf, " port='%s'", - src->hosts[n].port); - - if (src->hosts[n].transport) - virBufferAsprintf(buf, " transport='%s'", - virStorageNetHostTransportTypeToString(src->hosts[n].transport)); - - virBufferEscapeString(buf, " socket='%s'", - src->hosts[n].socket); - - virBufferAddLit(buf, "/>\n"); - } - - virBufferEscapeString(buf, "<snapshot name='%s'/>\n", - src->snapshot); - - virBufferEscapeString(buf, "<config file='%s'/>\n", - src->configFile); - - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</source>\n"); - } + if (virDomainDiskSourceFormatNetwork(buf, src) < 0) + return -1; break; case VIR_STORAGE_TYPE_VOLUME: -- 2.12.2
On Wed, Apr 26, 2017 at 19:52:38 +0200, Peter Krempa wrote:
--- src/conf/domain_conf.c | 99 ++++++++++++++++++++++++++------------------------ 1 file changed, 52 insertions(+), 47 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
--- .../generic-disk-network-http.xml | 44 ++++++++++++++++++++++ tests/genericxml2xmltest.c | 1 + 2 files changed, 45 insertions(+) create mode 100644 tests/genericxml2xmlindata/generic-disk-network-http.xml diff --git a/tests/genericxml2xmlindata/generic-disk-network-http.xml b/tests/genericxml2xmlindata/generic-disk-network-http.xml new file mode 100644 index 000000000..51c779502 --- /dev/null +++ b/tests/genericxml2xmlindata/generic-disk-network-http.xml @@ -0,0 +1,44 @@ +<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='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='http' name='test.img'> + <host name='example.org'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='https' name='test2.img'> + <host name='example.org'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='http' name='test3.img'> + <host name='example.org' port='1234'/> + </source> + <target dev='vdc' bus='virtio'/> + </disk> + <controller type='usb' 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/genericxml2xmltest.c b/tests/genericxml2xmltest.c index 1cda18cd9..49c692ffd 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -99,6 +99,7 @@ mymain(void) DO_TEST("perf"); DO_TEST("vcpus-individual"); + DO_TEST("disk-network-http"); virObjectUnref(caps); virObjectUnref(xmlopt); -- 2.12.2
On Wed, Apr 26, 2017 at 19:52:39 +0200, Peter Krempa wrote:
--- .../generic-disk-network-http.xml | 44 ++++++++++++++++++++++ tests/genericxml2xmltest.c | 1 + 2 files changed, 45 insertions(+) create mode 100644 tests/genericxml2xmlindata/generic-disk-network-http.xml
diff --git a/tests/genericxml2xmlindata/generic-disk-network-http.xml b/tests/genericxml2xmlindata/generic-disk-network-http.xml new file mode 100644 index 000000000..51c779502 --- /dev/null +++ b/tests/genericxml2xmlindata/generic-disk-network-http.xml @@ -0,0 +1,44 @@ +<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='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='http' name='test.img'> + <host name='example.org'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> ...
I'd think a <readonly/> element would be mandatory for http(s) disks, but apparently it isn't... This patch ends the part of useful refactors and cleanups and my review ends here too. Feel free to push everything I acked. The rest of the series is related to HTTP cookies and some design decisions need to be solved first. BTW, I tend to agree with Dan B. Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The helper returns true if a string contains any of the given chars. virStringHasControlChars can be reimplemented using that helper. --- src/util/virstring.c | 23 +++++++++++++++++++---- src/util/virstring.h | 2 ++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index 69abc267b..172df5496 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -1045,6 +1045,24 @@ virStringStripIPv6Brackets(char *str) } +/** + * virStringHasChars: + * @str: string to look for chars in + * @chars: chars to find in string @str + * + * Returns true if @str contains any of the chars in @chars. + */ +bool +virStringHasChars(const char *str, + const char *chars) +{ + if (!str) + return false; + + return str[strcspn(str, chars)] != '\0'; +} + + static const char control_chars[] = "\x01\x02\x03\x04\x05\x06\x07" "\x08" /* \t \n */ "\x0B\x0C" /* \r */ "\x0E\x0F" @@ -1054,10 +1072,7 @@ static const char control_chars[] = bool virStringHasControlChars(const char *str) { - if (!str) - return false; - - return str[strcspn(str, control_chars)] != '\0'; + return virStringHasChars(str, control_chars); } diff --git a/src/util/virstring.h b/src/util/virstring.h index 603650aa1..da7c9c2c1 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -280,6 +280,8 @@ char *virStringReplace(const char *haystack, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); void virStringStripIPv6Brackets(char *str); +bool virStringHasChars(const char *str, + const char *chars); bool virStringHasControlChars(const char *str); void virStringStripControlChars(char *str); -- 2.12.2
Add possibility to specify one or more cookies for http based disks. This patch adds the config parser, storage and validation of the cookies. --- docs/formatdomain.html.in | 9 ++ docs/schemas/domaincommon.rng | 39 +++++-- src/conf/domain_conf.c | 108 +++++++++++++++++- src/util/virstoragefile.c | 124 +++++++++++++++++++++ src/util/virstoragefile.h | 14 +++ .../generic-disk-network-http.xml | 4 + 6 files changed, 289 insertions(+), 9 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e31a271a5..ab70edff3 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2245,6 +2245,9 @@ <driver name='qemu' type='raw'/> <source protocol="http" name="url_path"> <host name="hostname" port="80"/> + <cookies> + <cookie name="test">somevalue</cookie> + </cookies> </source> <target dev='hde' bus='ide' tray='open'/> <readonly/> @@ -2593,6 +2596,12 @@ protocol. Supported for 'rbd' <span class="since">since 1.2.11 (QEMU only).</span> </dd> + <dt><code>cookies</code></dt> + <dd> + For <code>http</code> and <code>https</code> accessed storage it's + possible to pass one or more cookies. The cookie name and value + must conform to the HTTP specification. + </dd> </dl> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 0c51f5151..b2fa72381 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1556,16 +1556,39 @@ </element> </define> + <define name="diskSourceNetworkProtocolHTTPCookies"> + <element name="cookies"> + <oneOrMore> + <element name="cookie"> + <attribute name="name"> + <data type="string"> + <param name="pattern">[!#$%&'*+\-.0-9A-Z\^_`a-z|~]+</param> + </data> + </attribute> + <data type="string"> + <param name="pattern">[!#$%&'()*+\-./0-9:>=<?@A-Z\^_`\[\]a-z|~]+</param> + </data> + </element> + </oneOrMore> + <empty/> + </element> + </define> + <define name="diskSourceNetworkProtocolHTTP"> <element name="source"> - <attribute name="protocol"> - <choice> - <value>http</value> - <value>https</value> - </choice> - </attribute> - <attribute name="name"/> - <ref name="diskSourceNetworkHost"/> + <interleave> + <attribute name="protocol"> + <choice> + <value>http</value> + <value>https</value> + </choice> + </attribute> + <attribute name="name"/> + <ref name="diskSourceNetworkHost"/> + <optional> + <ref name="diskSourceNetworkProtocolHTTPCookies"/> + </optional> + </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5a736c853..b1a357174 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7520,6 +7520,78 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, } +static virStorageNetCookieDefPtr +virDomainStorageCookieParse(xmlNodePtr node, + xmlXPathContextPtr ctxt) +{ + virStorageNetCookieDefPtr cookie = NULL; + virStorageNetCookieDefPtr ret = NULL; + xmlNodePtr oldnode = ctxt->node; + + ctxt->node = node; + + if (VIR_ALLOC(cookie) < 0) + goto cleanup; + + if (!(cookie->name = virXPathString("string(./@name)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing cookie name")); + goto cleanup; + } + + if (!(cookie->value = virXPathString("string(.)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, _("missing value for cookie '%s'"), + cookie->name); + goto cleanup; + } + + VIR_STEAL_PTR(ret, cookie); + + cleanup: + ctxt->node = oldnode; + virStorageNetCookieDefFree(cookie); + return ret; +} + + +static int +virDomainStorageCookiesParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virStorageSourcePtr src) +{ + xmlNodePtr oldnode = ctxt->node; + xmlNodePtr *nodes = NULL; + ssize_t nnodes; + size_t i; + int ret = -1; + + ctxt->node = node; + + if ((nnodes = virXPathNodeSet("./cookie", ctxt, &nodes)) < 0) + goto cleanup; + + if (VIR_ALLOC_N(src->cookies, nnodes) < 0) + goto cleanup; + + src->ncookies = nnodes; + + for (i = 0; i < nnodes; i++) { + if (!(src->cookies[i] = virDomainStorageCookieParse(nodes[i], ctxt))) + goto cleanup; + } + + if (virStorageSourceNetCookiesValidate(src) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(nodes); + ctxt->node = oldnode; + + return ret; +} + + int virDomainDiskSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -7528,6 +7600,7 @@ virDomainDiskSourceParse(xmlNodePtr node, int ret = -1; char *protocol = NULL; xmlNodePtr saveNode = ctxt->node; + xmlNodePtr tmpnode; ctxt->node = node; @@ -7590,6 +7663,12 @@ virDomainDiskSourceParse(xmlNodePtr node, if (virDomainStorageHostParse(node, &src->hosts, &src->nhosts) < 0) goto cleanup; + + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_HTTP && + (tmpnode = virXPathNode("./cookies", ctxt))) { + if (virDomainStorageCookiesParse(tmpnode, ctxt, src) < 0) + goto cleanup; + } break; case VIR_STORAGE_TYPE_VOLUME: if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0) @@ -20769,6 +20848,30 @@ virDomainSourceDefFormatSeclabel(virBufferPtr buf, static int +virDomainDiskSourceFormatNetworkCookies(virBufferPtr buf, + virStorageSourcePtr src) +{ + size_t i; + + if (src->ncookies == 0) + return 0; + + virBufferAddLit(buf, "<cookies>\n"); + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < src->ncookies; i++) { + virBufferEscapeString(buf, "<cookie name='%s'>", src->cookies[i]->name); + virBufferEscapeString(buf, "%s</cookie>\n", src->cookies[i]->value); + } + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</cookies>\n"); + + return 0; +} + + +static int virDomainDiskSourceFormatNetwork(virBufferPtr buf, virStorageSourcePtr src) { @@ -20787,7 +20890,7 @@ virDomainDiskSourceFormatNetwork(virBufferPtr buf, VIR_FREE(path); - if (src->nhosts == 0 && !src->snapshot && !src->configFile) { + if (src->nhosts == 0 && !src->snapshot && !src->configFile && src->ncookies == 0) { virBufferAddLit(buf, "/>\n"); } else { virBufferAddLit(buf, ">\n"); @@ -20809,6 +20912,9 @@ virDomainDiskSourceFormatNetwork(virBufferPtr buf, virBufferEscapeString(buf, "<snapshot name='%s'/>\n", src->snapshot); virBufferEscapeString(buf, "<config file='%s'/>\n", src->configFile); + if (virDomainDiskSourceFormatNetworkCookies(buf, src) < 0) + return -1; + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</source>\n"); } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 94a77ce86..fe03a3009 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1948,6 +1948,126 @@ virStorageSourceSeclabelsCopy(virStorageSourcePtr to, } +void +virStorageNetCookieDefFree(virStorageNetCookieDefPtr def) +{ + if (!def) + return; + + VIR_FREE(def->name); + VIR_FREE(def->value); + + VIR_FREE(def); +} + + +static void +virStorageSourceCookiesClear(virStorageSourcePtr src) +{ + size_t i; + + if (!src || !src->cookies) + return; + + for (i = 0; i < src->ncookies; i++) + virStorageNetCookieDefFree(src->cookies[i]); + + VIR_FREE(src->cookies); + src->ncookies = 0; +} + + +static int +virStorageSourceNetCookiesCopy(virStorageSourcePtr to, + const virStorageSource *from) +{ + size_t i; + + if (from->ncookies == 0) + return 0; + + if (VIR_ALLOC_N(to->cookies, from->ncookies) < 0) + return -1; + to->ncookies = from->ncookies; + + for (i = 0; i < from->ncookies; i++) { + if (VIR_STRDUP(to->cookies[i]->name, from->cookies[i]->name) < 0 || + VIR_STRDUP(to->cookies[i]->value, from->cookies[i]->value) < 0) + goto error; + } + + return 0; + + error: + virStorageSourceCookiesClear(to); + return -1; +} + + +/* see https://tools.ietf.org/html/rfc6265#section-4.1.1 */ +static const char virStorageSourceCookieValueInvalidChars[] = + "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F" + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1A\x1B\x1C\x1D\x1E\x1F" + " \",;\\"; + +/* in addition cookie name can't contain these */ +static const char virStorageSourceCookieNameInvalidChars[] = + "()<>@:/[]?={}"; + +static int +virStorageSourceNetCookieValidate(virStorageNetCookieDefPtr def) +{ + /* name must have at least 1 character */ + if (*(def->name) == '\0') { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cookie name must not be empty")); + return -1; + } + + /* check invalid characters in name */ + if (virStringHasChars(def->name, virStorageSourceCookieValueInvalidChars) || + virStringHasChars(def->name, virStorageSourceCookieNameInvalidChars)) { + virReportError(VIR_ERR_XML_ERROR, + _("cookie name '%s' contains invalid characters"), + def->name); + return -1; + } + + /* check invalid characters in value */ + if (virStringHasChars(def->value, virStorageSourceCookieValueInvalidChars)) { + virReportError(VIR_ERR_XML_ERROR, + _("value of cookie '%s' contains invalid characters"), + def->name); + return -1; + } + + return 0; +} + + +int +virStorageSourceNetCookiesValidate(virStorageSourcePtr src) +{ + size_t i; + size_t j; + + for (i = 0; i < src->ncookies; i++) { + if (virStorageSourceNetCookieValidate(src->cookies[i]) < 0) + return -1; + + for (j = i + 1; j < src->ncookies; j++) { + if (STREQ(src->cookies[i]->name, src->cookies[j]->name)) { + virReportError(VIR_ERR_XML_ERROR, _("duplicate cookie '%s'"), + src->cookies[i]->name); + return -1; + } + } + } + + return 0; +} + + static virStorageTimestampsPtr virStorageTimestampsCopy(const virStorageTimestamps *src) { @@ -2060,6 +2180,9 @@ virStorageSourceCopy(const virStorageSource *src, ret->nhosts = src->nhosts; } + if (virStorageSourceNetCookiesCopy(ret, src) < 0) + goto error; + if (src->srcpool && !(ret->srcpool = virStorageSourcePoolDefCopy(src->srcpool))) goto error; @@ -2258,6 +2381,7 @@ virStorageSourceClear(virStorageSourcePtr def) VIR_FREE(def->volume); VIR_FREE(def->snapshot); VIR_FREE(def->configFile); + virStorageSourceCookiesClear(def); virStorageSourcePoolDefFree(def->srcpool); VIR_FREE(def->driverName); virBitmapFree(def->features); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 9ebfc1108..42d9eac61 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -160,6 +160,16 @@ struct _virStorageNetHostDef { char *socket; /* path to unix socket */ }; +typedef struct _virStorageNetCookieDef virStorageNetCookieDef; +typedef virStorageNetCookieDef *virStorageNetCookieDefPtr; +struct _virStorageNetCookieDef { + char *name; + char *value; +}; + +void virStorageNetCookieDefFree(virStorageNetCookieDefPtr def); + + /* Information for a storage volume from a virStoragePool */ /* @@ -235,6 +245,8 @@ struct _virStorageSource { the source definition */ size_t nhosts; virStorageNetHostDefPtr hosts; + size_t ncookies; + virStorageNetCookieDefPtr *cookies; virStorageSourcePoolDefPtr srcpool; virStorageAuthDefPtr auth; virStorageEncryptionPtr encryption; @@ -371,6 +383,8 @@ int virStorageSourceUpdateCapacity(virStorageSourcePtr src, char *buf, ssize_t len, bool probe); +int virStorageSourceNetCookiesValidate(virStorageSourcePtr src); + virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, bool backingChain) diff --git a/tests/genericxml2xmlindata/generic-disk-network-http.xml b/tests/genericxml2xmlindata/generic-disk-network-http.xml index 51c779502..fc5ac6e5e 100644 --- a/tests/genericxml2xmlindata/generic-disk-network-http.xml +++ b/tests/genericxml2xmlindata/generic-disk-network-http.xml @@ -32,6 +32,10 @@ <driver name='qemu' type='raw'/> <source protocol='http' name='test3.img'> <host name='example.org' port='1234'/> + <cookies> + <cookie name='test'>testcookievalue</cookie> + <cookie name='test2'>blurb</cookie> + </cookies> </source> <target dev='vdc' bus='virtio'/> </disk> -- 2.12.2
On Wed, Apr 26, 2017 at 19:52:41 +0200, Peter Krempa wrote:
Add possibility to specify one or more cookies for http based disks. This patch adds the config parser, storage and validation of the cookies. --- docs/formatdomain.html.in | 9 ++ docs/schemas/domaincommon.rng | 39 +++++-- src/conf/domain_conf.c | 108 +++++++++++++++++- src/util/virstoragefile.c | 124 +++++++++++++++++++++ src/util/virstoragefile.h | 14 +++ .../generic-disk-network-http.xml | 4 + 6 files changed, 289 insertions(+), 9 deletions(-)
[...]
@@ -7590,6 +7663,12 @@ virDomainDiskSourceParse(xmlNodePtr node,
if (virDomainStorageHostParse(node, &src->hosts, &src->nhosts) < 0) goto cleanup; + + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_HTTP &&
This is missing "|| src->protocol == VIR_STORAGE_NET_PROTOCOL_HTTPS". Consider this fixed in the private branch.
+ (tmpnode = virXPathNode("./cookies", ctxt))) { + if (virDomainStorageCookiesParse(tmpnode, ctxt, src) < 0) + goto cleanup; + } break; case VIR_STORAGE_TYPE_VOLUME: if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0)
To allow formatting params for other protocols too add convert it to a switch statement. --- src/qemu/qemu_command.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1116d2cd5..e9c3ea952 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1528,10 +1528,27 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, } virBufferAddLit(buf, ","); - if (disk->src->type == VIR_STORAGE_TYPE_NETWORK && - disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) { - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_GLUSTER_DEBUG_LEVEL)) - virBufferAsprintf(buf, "file.debug=%d,", cfg->glusterDebugLevel); + if (disk->src->type == VIR_STORAGE_TYPE_NETWORK) { + switch ((virStorageNetProtocol) disk->src->protocol) { + case VIR_STORAGE_NET_PROTOCOL_NONE: + case VIR_STORAGE_NET_PROTOCOL_NBD: + case VIR_STORAGE_NET_PROTOCOL_RBD: + case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: + case VIR_STORAGE_NET_PROTOCOL_ISCSI: + case VIR_STORAGE_NET_PROTOCOL_HTTP: + case VIR_STORAGE_NET_PROTOCOL_HTTPS: + case VIR_STORAGE_NET_PROTOCOL_FTP: + case VIR_STORAGE_NET_PROTOCOL_FTPS: + case VIR_STORAGE_NET_PROTOCOL_TFTP: + case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_LAST: + break; + + case VIR_STORAGE_NET_PROTOCOL_GLUSTER: + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_GLUSTER_DEBUG_LEVEL)) + virBufferAsprintf(buf, "file.debug=%d,", cfg->glusterDebugLevel); + break; + } } if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { -- 2.12.2
Add a capability that will cover 'sslverify', 'timeout' and 'cookie' which was recently added after qemu 2.9 finally fixed the introspection for the curl driver. --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 3 files changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index cc3e1f808..959606565 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -364,6 +364,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "query-cpu-definitions", /* 250 */ "block-write-threshold", "query-named-block-nodes", + "block-curl-options", ); @@ -1718,6 +1719,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsUSBNECXHCI[] = { static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = { { "blockdev-add/arg-type/options/+gluster/debug-level", QEMU_CAPS_GLUSTER_DEBUG_LEVEL}, { "blockdev-add/arg-type/+gluster/debug", QEMU_CAPS_GLUSTER_DEBUG_LEVEL}, + { "blockdev-add/arg-type/+http/cookie", QEMU_CAPS_BLOCK_CURL_OPTIONS}, }; struct virQEMUCapsObjectTypeProps { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index a7eec52a9..049829839 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -401,6 +401,7 @@ typedef enum { QEMU_CAPS_QUERY_CPU_DEFINITIONS, /* qmp query-cpu-definitions */ QEMU_CAPS_BLOCK_WRITE_THRESHOLD, /* BLOCK_WRITE_THRESHOLD event */ QEMU_CAPS_QUERY_NAMED_BLOCK_NODES, /* qmp query-named-block-nodes */ + QEMU_CAPS_BLOCK_CURL_OPTIONS, /* sslverify, cookie, timeout */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml index d238078af..c3e05f6a6 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml @@ -208,6 +208,7 @@ <flag name='query-cpu-definitions'/> <flag name='block-write-threshold'/> <flag name='query-named-block-nodes'/> + <flag name='block-curl-options'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <package> (v2.9.0)</package> -- 2.12.2
Format the string into the "curl" format so that it's accepted by qemu. Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140164 --- src/qemu/qemu_command.c | 27 +++++++++++- .../qemuxml2argv-disk-drive-network-http.args | 32 +++++++++++++++ .../qemuxml2argv-disk-drive-network-http.xml | 48 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e9c3ea952..980559859 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1467,6 +1467,7 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; virJSONValuePtr srcprops = NULL; char *source = NULL; + size_t i; int ret = -1; if (qemuGetDriveSourceProps(disk->src, &srcprops) < 0) @@ -1535,8 +1536,6 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, case VIR_STORAGE_NET_PROTOCOL_RBD: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_ISCSI: - case VIR_STORAGE_NET_PROTOCOL_HTTP: - case VIR_STORAGE_NET_PROTOCOL_HTTPS: case VIR_STORAGE_NET_PROTOCOL_FTP: case VIR_STORAGE_NET_PROTOCOL_FTPS: case VIR_STORAGE_NET_PROTOCOL_TFTP: @@ -1544,6 +1543,30 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, case VIR_STORAGE_NET_PROTOCOL_LAST: break; + case VIR_STORAGE_NET_PROTOCOL_HTTP: + case VIR_STORAGE_NET_PROTOCOL_HTTPS: + if (disk->src->ncookies > 0) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCK_CURL_OPTIONS)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu does not support http cookies")); + goto cleanup; + } + + virBufferAddLit(buf, "file.cookie="); + for (i = 0; i < disk->src->ncookies; i++) { + if (i > 0) + virBufferAddLit(buf, "; "); + + virBufferAsprintf(buf, "%s=%s", + disk->src->cookies[i]->name, + disk->src->cookies[i]->value); + } + + virBufferAddLit(buf, ","); + } + + break; + case VIR_STORAGE_NET_PROTOCOL_GLUSTER: if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_GLUSTER_DEBUG_LEVEL)) virBufferAsprintf(buf, "file.debug=%d,", cfg->glusterDebugLevel); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.args new file mode 100644 index 000000000..9900866cc --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.args @@ -0,0 +1,32 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=http://example.org:80/test.img,format=raw,if=none,\ +id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-drive file=https://example.org:443/test2.img,format=raw,if=none,\ +id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk1,\ +id=virtio-disk1 \ +-drive 'file=http://example.org:1234/test3.img,\ +file.cookie=test=testcookievalue; test2=blurb,format=raw,if=none,\ +id=drive-virtio-disk2' \ +-device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk2,\ +id=virtio-disk2 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.xml new file mode 100644 index 000000000..fc5ac6e5e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.xml @@ -0,0 +1,48 @@ +<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='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='http' name='test.img'> + <host name='example.org'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='https' name='test2.img'> + <host name='example.org'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='http' name='test3.img'> + <host name='example.org' port='1234'/> + <cookies> + <cookie name='test'>testcookievalue</cookie> + <cookie name='test2'>blurb</cookie> + </cookies> + </source> + <target dev='vdc' bus='virtio'/> + </disk> + <controller type='usb' 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/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c1b014b7d..fb49caa92 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -886,6 +886,7 @@ mymain(void) DO_TEST("disk-drive-network-iscsi-lun", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_BLOCK); + DO_TEST("disk-drive-network-http", QEMU_CAPS_BLOCK_CURL_OPTIONS); DO_TEST("disk-drive-network-gluster", QEMU_CAPS_GLUSTER_DEBUG_LEVEL); DO_TEST("disk-drive-network-rbd", NONE); -- 2.12.2
On Wed, Apr 26, 2017 at 07:52:44PM +0200, Peter Krempa wrote:
Format the string into the "curl" format so that it's accepted by qemu.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140164
[snip]
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.args new file mode 100644 index 000000000..9900866cc --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-http.args @@ -0,0 +1,32 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=http://example.org:80/test.img,format=raw,if=none,\ +id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-drive file=https://example.org:443/test2.img,format=raw,if=none,\ +id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk1,\ +id=virtio-disk1 \ +-drive 'file=http://example.org:1234/test3.img,\ +file.cookie=test=testcookievalue; test2=blurb,format=raw,if=none,\
Your example cookie is rather tame, but I wonder if we should consider cookie values to be security sensitive data, and thus use the secrets mechanism. If we did this would also entail fixes to QEMU to let use its secrets mechanism too. I'm just wary of re-introducing a bug like CVE-2015-5160 (rbd password information leak), via sensitive cookie values. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Apr 27, 2017 at 16:30:44 +0100, Daniel Berrange wrote:
On Wed, Apr 26, 2017 at 07:52:44PM +0200, Peter Krempa wrote:
Format the string into the "curl" format so that it's accepted by qemu.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140164
[snip]
Your example cookie is rather tame, but I wonder if we should consider cookie values to be security sensitive data, and thus use the secrets mechanism. If we did this would also entail fixes to QEMU to let use its secrets mechanism too.
I thought briefly about the same before posting this, but I went through anyways ...
I'm just wary of re-introducing a bug like CVE-2015-5160 (rbd password information leak), via sensitive cookie values.
We could allow generic cookies passed on the command line and then perhaps add a <cookie name="ble" secure='yes'>value</cookie> which will be passed via the secrets infrastructure. In that case I should probably add a statement saying that the cookies are passed in a insecure way., This way generic cookies can be passed even now and the provision for secure cookies can be added once qemu adds that feature. Peter
On Thu, Apr 27, 2017 at 17:46:16 +0200, Peter Krempa wrote:
On Thu, Apr 27, 2017 at 16:30:44 +0100, Daniel Berrange wrote:
On Wed, Apr 26, 2017 at 07:52:44PM +0200, Peter Krempa wrote:
Format the string into the "curl" format so that it's accepted by qemu.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140164
[snip]
Your example cookie is rather tame, but I wonder if we should consider cookie values to be security sensitive data, and thus use the secrets mechanism. If we did this would also entail fixes to QEMU to let use its secrets mechanism too.
I thought briefly about the same before posting this, but I went through anyways ...
I'm just wary of re-introducing a bug like CVE-2015-5160 (rbd password information leak), via sensitive cookie values.
We could allow generic cookies passed on the command line and then perhaps add a <cookie name="ble" secure='yes'>value</cookie> which will be passed via the secrets infrastructure.
Or better, treat them as secure by default and make users add secure='no' explicitly so that they are aware of the way we pass it.
On Thu, Apr 27, 2017 at 05:46:16PM +0200, Peter Krempa wrote:
On Thu, Apr 27, 2017 at 16:30:44 +0100, Daniel Berrange wrote:
On Wed, Apr 26, 2017 at 07:52:44PM +0200, Peter Krempa wrote:
Format the string into the "curl" format so that it's accepted by qemu.
Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140164
[snip]
Your example cookie is rather tame, but I wonder if we should consider cookie values to be security sensitive data, and thus use the secrets mechanism. If we did this would also entail fixes to QEMU to let use its secrets mechanism too.
I thought briefly about the same before posting this, but I went through anyways ...
I'm just wary of re-introducing a bug like CVE-2015-5160 (rbd password information leak), via sensitive cookie values.
We could allow generic cookies passed on the command line and then perhaps add a <cookie name="ble" secure='yes'>value</cookie> which will be passed via the secrets infrastructure.
In that case I should probably add a statement saying that the cookies are passed in a insecure way.,
This way generic cookies can be passed even now and the provision for secure cookies can be added once qemu adds that feature.
The thing is it feels like the compelling reason to use cookies in context of QEMU is precisely as an authorization mechanism. Even if we document them as "insecure" people will do it anyway, and the security flaw that results will be a libvirt CVE because we don't provide apps an alternative todo what they need. In addition, if the connection is using https: protocol, then I we think we should be doing encryption for all cookies, and not expect apps to set a secure=yes|no flag in the XML. Last time we accepted a temporary insecure solution we waited 5 years for QEMU to get us a fix... So I'm inclined to NACK this feature until QEMU provides us a way to handle cookies securely. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Peter Krempa