[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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.

> > 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.

> >> +++ 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.
> 
> 
> >> +    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.
> 
> 
> >> +++ 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.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]