[libvirt] [PATCH] qemu: configurable VNC port boundaries for domains

[Martin Kletzander]

On 05/22/2012 07:22 PM, Dave Allan wrote:
> On Tue, May 22, 2012 at 09:09:55AM -0600, Eric Blake wrote:
>> On 05/22/2012 09:00 AM, Dave Allan wrote:
>>> On Tue, May 22, 2012 at 04:10:03PM +0200, Martin Kletzander wrote:
>>>> The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find
>>>> free port when starting domains. As this was hardcoded to the same
>>>> ports as default VNC servers, there were races with these other
>>>> programs. This patch includes the possibility to change the default
>>>> starting port as well as the maximum port in qemu config file.
>>>
>>> Hi Martin,
>>>
>>> Two design comments:
>>>
>>> 1) https://bugzilla.redhat.com/show_bug.cgi?id=782814 requests that
>>> the default port be changed to avoid conflicts, which seems reasonable
>>> to me.
>>
>> If we choose better defaults for new installations, we still need to
>> worry about preserving existing ranges when upgrading old installations.
>>  This may need some coordination with the spec file doing some %post
>> magic to add in vnc_port_min with a value other than 5900 to qemu.conf
>> on new installations, but leaving it unspecified when doing an upgrade.
> 
> That's a good point, we certainly don't want to break things on
> upgrade.  At least one case that would is where people have opened the
> old range in a firewall, and now instead of ports, say, 5900-n, now
> people will be getting some other range.
> 

Yes, I agree, that's why I kept the defaults unchanged. But wouldn't it
be confusing for lot of users if we changed the defaults for new
installations? Because from user point of view, I think it would cause
misunderstanding "why on _some_ installations it uses different ports?".

Maybe I misunderstood what they meant in the BZ, but I though of the
default port as "the port the we start with when domain has specified
port -1", so they don't have to change it for all of the domains, but
just in one config file.

>>> 2) I agree with the config option since most applications on the
>>> system will want the system defaults.  However, IMO in this case an
>>> application writer should be given the option in the XML to override
>>> the system default.
>>
>> Agreed - I think we need both solutions - qemu.conf to specify the
>> default range, and per-domain XML to specify an override (does the XML
>> need to specify a range, or just a single port?).
> 
> I think a range, like the config option.
> 

This is maybe another misunderstanding on my side, but I believed that
when user specifies a port in the domain XML (e.g. port='12345'), he can
still configure autoport='yes', thus keeping the searching mechanism
working the same way as with the port='-1' option, just changing the
starting port. But I'm not sure about that, I have to look into this now.

However, I see two possibilities that seem reasonable:
 1) if autoport doesn't work as I thought, I can make it work as
explained before, I think it's pretty intuitive to make it like that
 2) independently on how the autoport works, I can make the range
configurable in the <listen> subelement or as an another option in the
<graphics> element (say: port_max, for example), as Dave mentioned.

>>>> +++ b/src/qemu/qemu_command.h
>>>> @@ -37,6 +37,9 @@
>>>>  # define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial"
>>>>  # define QEMU_FSDEV_HOST_PREFIX "fsdev-"
>>>>
>>>> +/* These are only defaults, they can be changed now in qemu.conf and
>>>> + * explicitely specified port is checked against these two (makes
>>
>> s/explicitely/explicitly/
>>
>>
>>>>
>>>> +    p = virConfGetValue (conf, "vnc_port_min");
>>>> +    CHECK_TYPE ("vnc_port_min", VIR_CONF_LONG);
>>>> +    if (p) {
>>>> +        if (p->l < QEMU_VNC_PORT_MIN) {
>>>> +            /* if the port is too low, we can't get the display name
>>>> +             * tell to vnc (usually subtract 5900, e.g. localhost:1
>>>> +             * for port 5901) */
>>>> +            qemuReportError(VIR_ERR_INTERNAL_ERROR,
>>>> +                            _("%s: vnc_port_min: port must be greater than or equal to %d"),
>>>> +                            filename, QEMU_VNC_PORT_MIN);
>>
>> You ought to mention this restriction in qemu.conf.

Yes, I'll do that, to explain there as well.

>>
>>
>>>> +    p = virConfGetValue (conf, "vnc_port_max");
>>>> +    CHECK_TYPE ("vnc_port_max", VIR_CONF_LONG);
>>>> +    if (p) {
>>>> +        if (p->l > QEMU_VNC_PORT_MAX ||
>>>> +            p->l <= driver->vncPortMin) {
>>
>> Off-by-one.  You want to allow min==max for specifying a range of
>> exactly 1 port, useful if you are only running one guest and want to
>> guarantee its port.  You need p->l < driver->vncPortMin, not p->l <=
>> driver->vncPortMin.

Actually, in case we specify min==max, there are few places where it
will fail, because we use it as (i = min; i < max; i++), but I'll change
that, because your version makes way more sense.

>>
>>
>>>> +++ b/src/qemu/qemu_conf.h
>>>> @@ -89,6 +89,8 @@ struct qemud_driver {
>>>>      unsigned int vncSASL : 1;
>>>>      char *vncTLSx509certdir;
>>>>      char *vncListen;
>>>> +    long vncPortMin;
>>>> +    long vncPortMax;
>>
>> long?  We know it's only 16 bits; short would do, int might be easier to
>> work with, but long is overkill.

OK, will switch that, I think I just wanted to keep it consistent with
the CHECK_TYPE, but that's unnecessary (or I don't know why I did that).

Martin