Re: [libvirt] [PATCH 2/2] Add support for QEMU -add-fd support detection

On 01/31/2013 04:46 AM, Stefan Berger wrote: > On 01/30/2013 11:21 PM, Eric Blake wrote: >> From: Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; >> >> Add support for QEMU -add-fd command line parameter detection. >> This intentionally rejects qemu 1.2, where 'add-fd' QMP did >> not allow full control of set ids, and where there was no command >> line counterpart, but accepts qemu 1.3. >> >> Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; >> --- >> + else if (STREQ(name, "add-fd")) >> + qemuCapsSet(caps, QEMU_CAPS_ADD_FD); > > This checks for QMP support -> version 1.2. Yes, but then... > >> VIR_FREE(name); >> } >> VIR_FREE(commands); >> >> + /* QMP add-fd was introduced in 1.2, but did not support >> + * management control of set numbering, and did not have a >> + * counterpart -add-fd command line option. We require the >> + * add-fd features from 1.3 or later. */ >> + if (qemuCapsGet(caps, QEMU_CAPS_ADD_FD)) { >> + int fd = open("/dev/null", O_RDONLY); >> + if (fd < 0) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >> + _("unable to probe for add-fd")); >> + return -1; >> + } >> + if (qemuMonitorAddFd(mon, 0, fd, "/dev/null") < 0) >> + qemuCapsClear(caps, QEMU_CAPS_ADD_FD); >> + VIR_FORCE_CLOSE(fd); >> + } >> + > > Aren't you only detecting version 1.2 with this? ...this code is what checks for 1.3. In 1.2, the qemuMonitorAddFd() call fails with an error, so we clear the bit back out. Only 1.3 allows libvirt to specify its own set number, because that was one of the changes qemu had to make in order to add the -add-fd command-line option.

> > I thought we would need something along the lines of this example here: > > if (version >= 9000) > qemuCapsSet(caps, QEMU_CAPS_VNC_COLON); That was my original idea, but a version number check is more fragile (it doesn't backport well); anywhere that we can do a feature check instead of a version check, we make backporting easier. -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list