[libvirt] [PATCHv3 1/2] conf: add <listen> subelement to domain <graphics> element
Eric Blake
eblake at redhat.com
Wed Jul 27 00:09:50 UTC 2011
On 07/25/2011 03:00 AM, Laine Stump wrote:
> Once it's plugged in, the<listen element will be an optional
> replacement for the "listen", "port", "tlsPort", and "autoport"
> attributes that graphics elements already have. If the<listen>
> type='address', it will have an attribute called 'address' which will
> contain an IP address or dns name that the guest's display server
> should listen on. If, however, type='network', the<listen> element
> should have an attribute called 'network' that will be set to the name
> of a network configuration to get the IP address from.
>
> 91 files changed, 1274 insertions(+), 343 deletions(-)
Big diff, but like you said mostly mechanical.
> @@ -2110,6 +2116,70 @@ qemu-kvm -net nic,model=? /dev/null
> </dd>
> </dl>
>
> +<p>
> + Rather than putting the information used to setup the listening
setup is a noun, but here you want a verb form:
s/setup/set up/
> +<dt><code>autoport</code></dt>
> +<dd>If set to 'yes', a listen port will be determined
> + automatically at runtime, and reflected in the domain's live
> + XML.
> +</dd>
May need tweaks depending on the decision on where autoport should live.
> @@ -1500,6 +1503,49 @@
> </choice>
> </element>
> </define>
> +
> +<define name="listenElements">
> +<zeroOrMore>
> +<element name="listen">
> +<optional>
> +<attribute name="type">
> +<choice>
> +<value>address</value>
> +<value>network</value>
/me curses thunderbird for squashing whitespace in my reply
I'm not sure this is right. I think we want to require either
type=address (which implies address=nnn must also be present, and
network=nnn must not be present) or type=network (which implies that
network=nnn must be present, and address=nnn might show up in live XML
dumpxml output to show what was resolved, but is ignored in input
parsing. Also, autoport=yes and port=nnn should be a forbidden
combination. That is, I think this should be as follows (assuming we
keep autoport in <listen>, further changes if autoport is global only):
<define name="listenElements">
<zeroOrMore>
<element name="listen">
<choice>
<group>
<attribute name="type">
<value>address</value>
</attribute>
<attribute name="address">
<ref name="addrIPorName"/>
</attribute>
</group>
<group>
<attribute name="type">
<value>network</value>
</attribute>
<attribute name="network">
<text/>
</attribute>
<optional>
<attribute name="address">
<ref name="addrIPorName"/>
</attribute>
</optional>
</group>
</choice>
<choice>
<attribute name="autoport">
<value>yes</value>
</attribute>
<group>
<optional>
<attribute name="autoport">
<value>no</value>
</attribute>
</optional>
<optional>
<attribute name="port">
<ref name="PortNumber"/>
</attribute>
</optional>
<optional>
<attribute name="tlsPort">
<ref name="PortNumber"/>
</attribute>
</optional>
</group>
</choice>
</element>
> +</zeroOrMore>
> +</define>
Two spaces too much indentation on </define> (not like it shows up any
better in my nitpick, though).
> @@ -4010,19 +4029,118 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
> return 0;
> }
>
> +static int
> +virDomainGraphicsListenParseXML(virDomainGraphicsListenDefPtr def,
> + xmlNodePtr node,
> + enum virDomainGraphicsType graphicsType,
> + unsigned int flags)
> +{
> + int ret = -1;
> + char *type = virXMLPropString(node, "type");
> + char *address = virXMLPropString(node, "address");
> + char *network = virXMLPropString(node, "network");
> + char *port = virXMLPropString(node, "port");
> + char *tlsPort = virXMLPropString(node, "tlsPort");
> + char *autoport = virXMLPropString(node, "autoport");
> +
> + if (type&& (def->type = virDomainGraphicsListenTypeFromString(type))< 0) {
> + virDomainReportError(VIR_ERR_XML_ERROR,
> + _("unknown graphics listen type '%s'"), type);
> + goto error;
> + }
Shouldn't this fail if type is not found? That is, I think type is a
mandatory attribute (must be address or network).
> +
> + if (address&& address[0]) {
> + def->address = address;
> + address = NULL;
Should we reject address if not VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS?
I learned with my virDomainSaveImageDefineXML patch that parsing must be
symmetric to output format. Since your output function produces address
for network mode on live xml but skips it for inactive, you _must_ make
the parser leave address as NULL on parsing if (flags &
VIR_DOMAIN_XML_INACTIVE) and network mode is detected.
> + }
> +
> + if (network&& network[0]) {
> + if (def->type != VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) {
> + virDomainReportError(VIR_ERR_XML_ERROR,
> + _("network attribute not allowed when listen type is not network"));
> + goto error;
> + }
> + def->network = network;
> + network = NULL;
> + }
Are we also missing checks that if type is address, then address was
specified; if type is network, then network was specified?
> +
> + if (port) {
> + if (virStrToLong_i(port, NULL, 10,&def->port)< 0) {
> + virDomainReportError(VIR_ERR_XML_ERROR,
> + _("cannot parse listen port %s"), port);
> + goto error;
> + }
> + /* legacy syntax for graphics type=vnc used -1 for
> + * autoport. We need to maintain it here because the legacy
> + * attributes in<graphics> must match those in
> + *<listen>. */
I'm not sure I buy this. We need to output the legacy attributes in
<graphics>, but <listen> is not constrained by legacy, so we can require
that if you use <listen> you only use the newer syntax.
> + if ((graphicsType == VIR_DOMAIN_GRAPHICS_TYPE_VNC)&&
> + (def->port == -1)) {
> + if (flags& VIR_DOMAIN_XML_INACTIVE)
> + def->port = 0;
> + def->autoport = true;
> + }
> + } else {
> + def->autoport = true;
> + }
> +
> + if (tlsPort&&
> + (graphicsType == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)) {
> + if (virStrToLong_i(tlsPort, NULL, 10,&def->tlsPort)< 0) {
> + virDomainReportError(VIR_ERR_XML_ERROR,
> + _("cannot parse spice tlsPort %s"), tlsPort);
> + goto error;
> + }
> + }
Should this error out if tlsPort but not GRAPHICS_TYPE_SPICE?
> +
> + if (autoport&& STREQ(autoport, "yes")) {
> + if (flags& VIR_DOMAIN_XML_INACTIVE) {
> + def->port = 0;
> + def->tlsPort = 0;
> + }
> + def->autoport = true;
We should check that autoport=yes and port are not simultaneously specified.
> + if (listenAddr&& STRNEQ_NULLABLE(listenAddr,
> + virDomainGraphicsListenGetAddress(def, 0))) {
> + virDomainReportError(VIR_ERR_XML_ERROR,
> + _("graphics listen attribute %s must match address "
> + "attribute of first listen element (found %s)"),
> + listenAddr, virDomainGraphicsListenGetAddress(def, 0));
> + goto error;
> + }
So what if I do:
<graphics port="5900">
<listen type="network" network="name" port="5901"/>
<listen type="address" address="127.0.0.1" port="5900"/>
</graphics>
Is that a valid setup, even though the legacy port doesn't match the
first <listen> port? That is, are we validating that the legacy must
match the first <listen>, or only the first <listen type=address>?
>
> +
> +static void
> +virDomainGraphicsListenDefFormat(virBufferPtr buf,
> + virDomainGraphicsListenDefPtr def,
> + enum virDomainGraphicsType graphicsType,
> + unsigned int flags)
> +{
> + virBufferAddLit(buf, "<listen");
> +
> + if (def->type) {
> + virBufferAsprintf(buf, " type='%s'",
> + virDomainGraphicsListenTypeToString(def->type));
> + }
> +
> + if (def->address&&
> + ((def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS)
> + || !(flags& VIR_DOMAIN_XML_INACTIVE))) {
> + /* address may also be set to show current status when type='network',
> + * but we don't want to print that if INACTIVE data is requested. */
> + virBufferAsprintf(buf, " address='%s'", def->address);
> + }
See the earlier comments - if you honor VIR_DOMAIN_XML_INACTIVE here to
control what gets output, you must also honor it on parsing to control
what gets skipped.
> +
> + if (def->network&&
> + (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)) {
> + virBufferAsprintf(buf, " network='%s'", def->network);
> + }
> +
> + /* For VNC definitions, if the INACTIVE XML was requested and
> + * autoport is on, we don't print output the port. (Not sure why
> + * the same thing isn't done for RDP and SPICE, but the existing
> + * code that this is mimicking doesn't do it.) */
Is that something we should do right, now that we're using new xml? I
think it boils down to:
In inactive xml, we want to know if the user specified a port
(autoport=no and port=nnn) or is okay with a default port (autoport=yes
and port can be omitted) - here, the autoport attribute seems redundant.
In active xml, we want to know if the user specified a port (autoport=no
and port=nnn), or was okay with a default port (autoport=yes) and which
port they got (port=nnn, or omitted if they have not got one yet).
> +static virDomainGraphicsListenDefPtr
> +virDomainGraphicsGetListen(virDomainGraphicsDefPtr def, size_t ii, bool force0)
> +{
> + if ((def->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) ||
> + (def->type == VIR_DOMAIN_GRAPHICS_TYPE_RDP) ||
> + (def->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE)) {
> +
> + if (!def->listens&& (ii == 0)&& force0) {
> + if (VIR_ALLOC(def->listens)< 0)
> + virReportOOMError();
This fails and leaves a message,
> + else
> + def->nListens = 1;
> + }
> +
> + if (!def->listens || (def->nListens<= ii))
> + return NULL;
but this fails without leaving a message. That can confuse callers (if
they got NULL, do they need to report a message, or has one already been
reported)? It might be better if you refactor things to always report
an error on failure here, except that...
> +
> +bool
> +virDomainGraphicsListenGetAutoport(virDomainGraphicsDefPtr def, size_t ii)
> +{
> + virDomainGraphicsListenDefPtr listenInfo
> + = virDomainGraphicsGetListen(def, ii, false);
> +
> + if (!listenInfo)
> + return false;
> + return listenInfo->autoport;
this is an example where you don't want an error message (returning
false is the right thing to do for out-of-bounds array)
> +}
> +
> +
> +int
> +virDomainGraphicsListenSetAutoport(virDomainGraphicsDefPtr def, size_t ii, bool val)
> +{
> + virDomainGraphicsListenDefPtr listenInfo
> + = virDomainGraphicsGetListen(def, ii, true);
> +
> + if (!listenInfo)
> + return -1;
whereas here, you want to know whether the -1 is because of an
allocation failure (message was reported, in the current code) or
because of a mismatch (<graphics> was not spice or vnc, so it has no
listen elements, no error message in the current code).
The rest of the patch looks fairly reasonable, so I think any remaining
effort is on ensuring a sane domain_conf.c parse and format of the new
attribute, how it interacts with autoport, and how the .rng file
reflects whatever restrictions are in the code.
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list