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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list