10:43 p.m.
On 03/09/2015 07:50 AM, John Ferlan wrote:
On 02/28/2015 04:08 AM, Luyao Huang wrote:
> If a interface or network have both ipv6 and ipv4 address which can be used,
> we do not know use which be a listen address. So introduce a new attribute
> to help us chose this.
>
> graphics XML will like this after this commit:
>
> <graphics type='spice' port='5900'
autoport='yes'>
> <listen type='network' address='192.168.0.1'
network='vepa-net' family='ipv4'/>
> </graphics>
>
> and this ip family can be set 2 type, and the default one is ipv4:
>
> ipv4: check if the interface or network have a can be used ipv4 address
> ipv6: check if the interface or network have a can be used ipv6 address
>
> fix some test which will be break by this commit and add a new test.
>
Adjusting the commit text slightly to match the review below
> Signed-off-by: Luyao Huang <lhuang(a)redhat.com>
> ---
> v2: remove family='default' and add a test.
>
> docs/formatdomain.html.in | 10 ++++++-
> docs/schemas/domaincommon.rng | 8 +++++
> src/conf/domain_conf.c | 20 +++++++++++++
> src/conf/domain_conf.h | 9 ++++++
> .../qemuxml2argv-graphics-listen-network-ipv6.xml | 35 ++++++++++++++++++++++
> .../qemuxml2argv-graphics-listen-network.xml | 2 +-
> .../qemuxml2xmlout-graphics-listen-network2.xml | 2 +-
> tests/qemuxml2xmltest.c | 1 +
> 8 files changed, 84 insertions(+), 3 deletions(-)
> create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index fb0a0d1..e8ea6fa 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4545,7 +4545,7 @@ qemu-kvm -net nic,model=? /dev/null
> <graphics type='rdp' autoport='yes'
multiUser='yes' />
> <graphics type='desktop' fullscreen='yes'/>
> <graphics type='spice'>
> - <listen type='network' network='rednet'/>
> + <listen type='network' network='rednet'
family='ipv4'/>
> </graphics>
> </devices>
> ...</pre>
> @@ -4785,6 +4785,14 @@ qemu-kvm -net nic,model=? /dev/null
> the first forward dev will be used.
> </dd>
> </dl>
> + <dl>
> + <dt><code>family</code></dt>
> + <dd>if <code>type='network'</code>, the
<code>family</code>
> + attribute will contain an IP family. This tells which IP address
> + will be got for the network. IP family can be set to ipv4
> + or ipv6.
Adjusted to:
<dd>if <code>type='network'</code>, the
<code>family</code>
attribute may contain the IP family. The <code>family</code>
can be set to either <code>ipv4</code> or
<code>ipv6</code>.
This advises the graphics device which IP address family
to use as listen address for the network. The listen address
used will be the first found address of the <code>family</code>
type defined for the host.
<span class="since">Since 1.2.14</span>
Looks good to me.
> + </dd>
> + </dl>
>
> <h4><a name="elementsVideo">Video
devices</a></h4>
> <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 4e4fe9f..e84b213 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2909,6 +2909,14 @@
> <ref name="addrIPorName"/>
> </attribute>
> </optional>
> + <optional>
> + <attribute name="family">
> + <choice>
> + <value>ipv4</value>
> + <value>ipv6</value>
> + </choice>
> + </attribute>
> + </optional>
> </group>
> </choice>
> </element>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9b7ae3f..97f1250 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -522,6 +522,11 @@ VIR_ENUM_IMPL(virDomainGraphicsListen,
VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST,
> "address",
> "network")
>
> +VIR_ENUM_IMPL(virDomainGraphicsListenFamily,
> + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST,
> + "ipv4",
> + "ipv6")
> +
Need to add a "none"... As with the previous review - adding an
attribute that's optional, but then generating it on output isn't good.
So we need a way to signify it didn't exist on input. If provided that's
fine, we can output it. If not provided, then sure we're going to
eventually default to IPv4, but that's why I said to have the boolean be
"if ipv6 desired"...
yes, thanks for pointing out, add a "none" is better than auto generate
it to ipv4.
Hmm, use family='ipv4|ipv6' in this place , another reason is : maybe
there will be a new ip family in the future (ipv7? :-P ), so use ip
family in this place will be a good choice in that day ( seems so far
away ;) )
> VIR_ENUM_IMPL(virDomainGraphicsAuthConnected,
> VIR_DOMAIN_GRAPHICS_AUTH_CONNECTED_LAST,
> "default",
> @@ -9464,6 +9469,7 @@
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
> char *address = virXMLPropString(node, "address");
> char *network = virXMLPropString(node, "network");
> char *fromConfig = virXMLPropString(node, "fromConfig");
> + char *family = virXMLPropString(node, "family");
> int tmp;
>
> if (!type) {
> @@ -9501,6 +9507,16 @@
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
> network = NULL;
> }
>
> + if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) {
> + if (!family) {
> + def->family = VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4;
strike this check
> + } else if ((def->family =
virDomainGraphicsListenFamilyTypeFromString(family)) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown graphics listen IP family
'%s'"), family);
> + goto error;
> + }
> + }
> +
> if (fromConfig &&
> flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
> if (virStrToLong_i(fromConfig, NULL, 10, &tmp) < 0) {
> @@ -9520,6 +9536,7 @@
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
> VIR_FREE(address);
> VIR_FREE(network);
> VIR_FREE(fromConfig);
> + VIR_FREE(family);
> return ret;
> }
>
> @@ -19084,6 +19101,9 @@ virDomainGraphicsListenDefFormat(virBufferPtr buf,
> if (def->network &&
> (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)) {
> virBufferEscapeString(buf, " network='%s'",
def->network);
> +
Add a condition
if (def->family != VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_NONE)
> + virBufferAsprintf(buf, " family='%s'",
> +
virDomainGraphicsListenFamilyTypeToString(def->family));
> }
>
> if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 02ddd93..afffe9c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1441,6 +1441,13 @@ typedef enum {
> } virDomainGraphicsListenType;
>
> typedef enum {
Add:
VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_NONE = 0,
> + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV4,
> + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_IPV6,
> +
> + VIR_DOMAIN_GRAPHICS_LISTEN_FAMILY_LAST
> +} virDomainGraphicsListenFamily;
> +
> +typedef enum {
> VIR_DOMAIN_HUB_TYPE_USB,
>
> VIR_DOMAIN_HUB_TYPE_LAST
> @@ -1453,6 +1460,7 @@ struct _virDomainGraphicsListenDef {
> char *address;
> char *network;
> bool fromConfig; /* true if the @address is config file originated */
> + int family; /*enum virDomainGraphicsListenFamily*/
> };
>
> struct _virDomainGraphicsDef {
> @@ -2866,6 +2874,7 @@ VIR_ENUM_DECL(virDomainInput)
> VIR_ENUM_DECL(virDomainInputBus)
> VIR_ENUM_DECL(virDomainGraphics)
> VIR_ENUM_DECL(virDomainGraphicsListen)
> +VIR_ENUM_DECL(virDomainGraphicsListenFamily)
> VIR_ENUM_DECL(virDomainGraphicsAuthConnected)
> VIR_ENUM_DECL(virDomainGraphicsSpiceChannelName)
> VIR_ENUM_DECL(virDomainGraphicsSpiceChannelMode)
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
> new file mode 100644
> index 0000000..6cce7a8
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network-ipv6.xml
> @@ -0,0 +1,35 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</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</emulator>
> + <disk type='block' device='disk'>
> + <source dev='/dev/HostVG/QEMUGuest1'/>
> + <target dev='hda' bus='ide'/>
> + <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
> + </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'/>
> + <graphics type='vnc' port='5903' autoport='no'>
> + <listen type='network' network='Bobsnetwork'
family='ipv6'/>
> + </graphics>
> + <video>
> + <model type='cirrus' vram='16384' heads='1'/>
> + </video>
> + <memballoon model='virtio'/>
> + </devices>
> +</domain>
Also create an -ipv4 specific file...
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
> index bf78ca8..3b5c2de 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-listen-network.xml
> @@ -25,7 +25,7 @@
> <input type='mouse' bus='ps2'/>
> <input type='keyboard' bus='ps2'/>
> <graphics type='vnc' port='5903' autoport='no'>
> - <listen type='network' network='Bobsnetwork'/>
> + <listen type='network' network='Bobsnetwork'
family='ipv4'/>
> </graphics>
> <video>
> <model type='cirrus' vram='16384' heads='1'/>
Revert this to having no change
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
> index abee7b6..2b2d78a 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-listen-network2.xml
> @@ -26,7 +26,7 @@
> <input type='keyboard' bus='ps2'/>
> <graphics type='vnc' port='-1' autoport='yes'
listen='1.2.3.4'>
> <listen type='address' address='1.2.3.4'/>
> - <listen type='network' network='Bobsnetwork'/>
> + <listen type='network' network='Bobsnetwork'
family='ipv4'/>
> </graphics>
> <video>
> <model type='cirrus' vram='16384' heads='1'/>
Revert to having no change
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 99d4629..c966eb5 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -248,6 +248,7 @@ mymain(void)
> DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE);
> DO_TEST_FULL("disk-active-commit", false, WHEN_ACTIVE);
> DO_TEST("graphics-listen-network");
Add
DO_TEST("graphics-listen-network-ipv4");
and of course the corresponding -ipv4.xml file.
Yes, need add a test for ipv4 and no need fix the existing tests if we
won't auto generate family='ipv4'.
Thanks for your review.
> + DO_TEST("graphics-listen-network-ipv6");
> DO_TEST("graphics-vnc");
> DO_TEST("graphics-vnc-websocket");
> DO_TEST("graphics-vnc-sasl");
>
John
Luyao