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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list