[libvirt] [PATCHv3 1/2] conf: add <listen> subelement to domain <graphics> element

Eric Blake eblake at redhat.com
Wed Jul 27 15:40:13 UTC 2011


On 07/27/2011 01:47 AM, Laine Stump wrote:
>> 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.
>
>
> Actually it's not forbidden, that's really the only practical use I can
> see for the autoport attribute - in live XML "autoport='yes' port='nnn'"
> is used to indicate that the port was auto-generated and this time 'nnn'
> was used. (and not that it's any indication of what is right or wrong,
> but the current RNG file allows it).
...
 >
 > Well, existing code doesn't do that. "Do no harm".
 >
 > Certainly in live XML this is acceptable, and for inactive XML there may
 > be cases where someone grabs live XML, modifies it, and sends the
 > results back to the parse as config (aka inactive).

Yeah, on thinking more, it would be nice if the rng file accepted all 
live xml (it doesn't, because of other aspects like <alias> and 
<actual>, but in general we tend to ignore rather than reject stuff we 
don't recognize).

So maybe your rng is okay as is - it accepts more than what is strictly 
necessary, but as long as it doesn't reject anything that the code 
accepts, we're okay.  The question comes down to whether it is easy to 
make the rng reject stuff that the code rejects, or whether to keep the 
rng simple and over-permissive.

>
> I think if we made autoport a single setting global to all instances of
> listen, and tried to prevent simultaneous (global) autoport and (local
> to one listen) port, the RNG would be quite unwieldy. (you would have to
> have a listenElementsPort and a listenElementsAutoport, and the graphics
> RNG would put the two of them in a <choice> depending on the setting of
> autoport)

Interesting point.  Overall, I think it boils down to whether we ever 
envision having more than one <listen>, where one specified a port and 
the other let the port be chosen automatically.  Conversely, if we start 
strict, we can always relax later (especially since it won't be till 
later that we support multiple <listen>), whereas if we start relaxed, 
it's harder to tighten things up.  But I'm starting to swing towards 
having autoport remain with <listen>.

>> Shouldn't this fail if type is not found? That is, I think type is a
>> mandatory attribute (must be address or network).
>
>
> No. It's also possible that no address/network information is given (in
> which case type would be "default", which ends up as just no type
> showing up at all). In that case, the qemu driver has default listen
> addresses for VNC and for Spice. So it's completely valid to give a
> port, but no address.

Makes sense.

So I think this can be expressed in rng as:

<optional>
   <choice>
     <group>
       <attribute>...type=address
       <attribute>...address=iporname
     </group>
     <group>
       <attribute>...type=network
       <attribute>...network=name
       <optional>
         <attribute>...address=ip
       </optional>
     </group>
   </choice>
</optional>

That is, type is optional, but if present, then it determines whether 
address or network must also be present.  Meanwhile, address can appear 
with either type, but if address appears, then type must appear.

>
>
>>
>>> +
>>> + if (address&& address[0]) {
>>> + def->address = address;
>>> + address = NULL;
>>
>> Should we reject address if not VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS?
>
>
> No. Although I haven't found the right place to put it in, it's possible
> that the listen type may be network, and the resolved address will be
> filled in so that it can show up as status in the live XML. (I actually
> originally had exactly that check put in, but removed it for this reason).

Yep, I agree that we want to be able to parse existing live output, even 
if we ignore that parse on INACTIVE.

>
>
>>
>> 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.
>
>
> I hadn't thought of that. But should we reject it in that case, or
> should we *ignore* it? (Will it ever be the case that someone will feed
> live XML back to the parser as config? And should we complain in that
> case, or silently ignore the extra, just as we would if it was something
> completely unknown to us?) (I usually tend to follow the IETF mantra of
> being lenient in what you accept, and strict in what you give out)

In general, we ignore (not reject) live-only information when doing an 
INACTIVE parse.  See for example how <alias> is treated - the format 
routines only output <alias> on live images, and when parsing INACTIVE, 
any existing <alias> is silently ignored.

> Possibly, I can modify the formatter to (1) never output port=-1 in
> listen, and 2) only output autoport in listen if we're doing live XML.
> Then on input, if <graphics has port=-1 that ends up being changed to
> port=0 + autoport=yes anyway, so the check to verify they match would
> still work.

Seems like a good plan of attack.

> Two points here:
>
> 1) I did say in earlier conversations that the attributes in <graphics>
> should match the first <listen> that has type='address'. I didn't have
> the time to implement it that way, though. Fortunately we currently have
> no driver that understands more than a single <listen> anyway, so this
> can be glossed over for the moment (as long as it's not forgotten).

If you don't address it now, be sure to leave some comments to that effect.

And if I haven't made it clear before, I'd like to see something like 
this patch make it into 0.9.4 - we've missed the rc1 freeze, but this 
only changes xml rather than adding new API, and it can be argued that 
this patch is necessary to round out a new feature being advertised for 
0.9.4 (that is, the feature of being able to isolate host-dependent 
network information out of domain xml is buggy without this patch).

>
> 2) Your example brings up an interesting question: the idea of allowing
> both a <listen> as well as the legacy attributes in <graphics> is really
> intended to ease compatibility with old applications that only
> understand the old attributes, and the duplication should *really* only
> be done by our formatter, not by anyone creating new XML. Our formatter
> would never copy port='5900' into <graphics> without also putting in
> listen='127.0.0.1' (since that's in the <listen> too. But the parser
> only checks that individual attributes match when specified in both
> places; it doesn't do a "if *any* attribute is specified in both places,
> they must *all* be specified in both places" check. Do we need to do
> that, or is it overkill? (this is a place where I can get on the "it's
> overkill" boat :-)

I think you've convinced me - pairwise comparison is simple enough to 
get this patch finished, and while entire set comparison would be 
stricter, it's probably overkill to worry about it now.
>> 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.
>
>
> Skipped, or forbidden? I've seen you suggest both in different (but
> similar) contexts.

You've convinced me - skip (not reject) anything that is valid as live 
xml but irrelevant as inactive xml (that is, autoport=yes + port=nnn on 
an INACTIVE parse takes only autoport=yes, and ignores port).  We can 
reserve rejections for something that will 1) never be output as live 
xml, and 2) would create an actual conflict (like trying to specify tls 
port and vnc).

>
> We may disagree about whether certain "extra stuff" should be ignored or
> rejected, though. I'll go through it again in the morning and see what
> falls out.

I definitely think I found a few things, but my original review mail is 
probably larger than the actual amount of changes you need to make in a v4.

>
> Thanks for the review!

One idea for splitting this into easier reviews (don't know how hard it 
would be though): in patch 1, implement the helper functions to get and 
set values, and convert all callers to use the helper functions, but 
where the helper functions are one-liner wrappers around direct access. 
  In patch 2, change domain_conf to support <listen> attributes, and 
update the helper functions, as well as update the test cases to react 
to the new formatted xml.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org




More information about the libvir-list mailing list