Re: [libvirt] [PATCH] qemu: configurable VNC port boundaries for domains
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.
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?).
> +++ 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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)