Hi, On 05/02/2014 06:44 AM, Peter Hutterer wrote: > We can only request one fd per device from systemd-logind. If a fd is re-used > by the same device, releasing the fd from one device doesn't mean we can close > it. The systemd code knows when it's really released, so let it close the fd. > > Test case: xorg.conf section for an input device with hotplugging enabled. > evdev detects the duplicate and closes the hotplugged device, which closes the > fd. The other instance of evdev thinks the fd is still valid so now you're > playing a double lottery. First, which client(s) will get the evdev fd? > Second, which requests will be picked up by evdev and which ones will be > picked up by the client? You'll never now, but the fun is in finding out. > > Signed-off-by: Peter Hutterer <[email protected]>
Thanks for catching this, looks good. One minor nitpick below. With that nitpick fixed this is: Reviewed-by: Hans de Goede <[email protected]> > --- > The first hunk could be reduced by an if statement but I found it more > obvious this way. > > config/config.c | 6 ++---- > hw/xfree86/common/xf86Xinput.c | 6 ++---- > hw/xfree86/common/xf86platformBus.c | 3 +-- > hw/xfree86/os-support/linux/lnx_platform.c | 2 +- > hw/xfree86/os-support/linux/systemd-logind.c | 7 +++++-- > include/systemd-logind.h | 4 ++-- > 6 files changed, 13 insertions(+), 15 deletions(-) > > diff --git a/config/config.c b/config/config.c > index def7f16..5514516 100644 > --- a/config/config.c > +++ b/config/config.c > @@ -250,8 +250,6 @@ config_odev_free_attributes(struct OdevAttributes > *attribs) > free(iter); > } > > - if (fd != -1) { > - systemd_logind_release_fd(major, minor); > - close(fd); > - } > + if (fd != -1) > + systemd_logind_release_fd(major, minor, fd); > } > diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c > index bc6b73f..220790d 100644 > --- a/hw/xfree86/common/xf86Xinput.c > +++ b/hw/xfree86/common/xf86Xinput.c > @@ -771,8 +771,7 @@ xf86DeleteInput(InputInfoPtr pInp, int flags) > FreeInputAttributes(pInp->attrs); > > if (pInp->flags & XI86_SERVER_FD) { > - systemd_logind_release_fd(pInp->major, pInp->minor); > - close(pInp->fd); > + systemd_logind_release_fd(pInp->major, pInp->minor, pInp->fd); > } > > /* Remove the entry from the list. */ Please remove the now superfluous curly braces. Regards, Hans > @@ -873,8 +872,7 @@ xf86NewInputDevice(InputInfoPtr pInfo, DeviceIntPtr > *pdev, BOOL enable) > sizeof(pInfo) * (new_input_devices_count + 1)); > new_input_devices[new_input_devices_count] = pInfo; > new_input_devices_count++; > - systemd_logind_release_fd(pInfo->major, pInfo->minor); > - close(fd); > + systemd_logind_release_fd(pInfo->major, pInfo->minor, fd); > return BadMatch; > } > pInfo->fd = fd; > diff --git a/hw/xfree86/common/xf86platformBus.c > b/hw/xfree86/common/xf86platformBus.c > index 4e80f9e..dd118a2 100644 > --- a/hw/xfree86/common/xf86platformBus.c > +++ b/hw/xfree86/common/xf86platformBus.c > @@ -340,8 +340,7 @@ static Bool doPlatformProbe(struct xf86_platform_device > *dev, DriverPtr drvp, > fd = xf86_get_platform_device_int_attrib(dev, ODEV_ATTRIB_FD, > -1); > major = xf86_get_platform_device_int_attrib(dev, > ODEV_ATTRIB_MAJOR, 0); > minor = xf86_get_platform_device_int_attrib(dev, > ODEV_ATTRIB_MINOR, 0); > - systemd_logind_release_fd(major, minor); > - close(fd); > + systemd_logind_release_fd(major, minor, fd); > config_odev_add_int_attribute(dev->attribs, ODEV_ATTRIB_FD, -1); > dev->flags &= ~XF86_PDEV_SERVER_FD; > } > diff --git a/hw/xfree86/os-support/linux/lnx_platform.c > b/hw/xfree86/os-support/linux/lnx_platform.c > index dbd7aa0..308275a 100644 > --- a/hw/xfree86/os-support/linux/lnx_platform.c > +++ b/hw/xfree86/os-support/linux/lnx_platform.c > @@ -37,7 +37,7 @@ get_drm_info(struct OdevAttributes *attribs, char *path, > int delayed_index) > if (paused) { > LogMessage(X_ERROR, > "Error systemd-logind returned paused fd for drm > node\n"); > - systemd_logind_release_fd(major, minor); > + systemd_logind_release_fd(major, minor, -1); > return FALSE; > } > config_odev_add_int_attribute(attribs, ODEV_ATTRIB_FD, fd); > diff --git a/hw/xfree86/os-support/linux/systemd-logind.c > b/hw/xfree86/os-support/linux/systemd-logind.c > index ed670a8..73a8d55 100644 > --- a/hw/xfree86/os-support/linux/systemd-logind.c > +++ b/hw/xfree86/os-support/linux/systemd-logind.c > @@ -162,7 +162,7 @@ cleanup: > } > > void > -systemd_logind_release_fd(int _major, int _minor) > +systemd_logind_release_fd(int _major, int _minor, int fd) > { > struct systemd_logind_info *info = &logind_info; > InputInfoPtr pInfo; > @@ -174,7 +174,7 @@ systemd_logind_release_fd(int _major, int _minor) > int matches = 0; > > if (!info->session || major == 0) > - return; > + goto close; > > /* Only release the fd if there is only 1 InputInfo left for this major > * and minor, otherwise other InputInfo's are still referencing the fd. > */ > @@ -218,6 +218,9 @@ cleanup: > if (reply) > dbus_message_unref(reply); > dbus_error_free(&error); > +close: > + if (fd != -1) > + close(fd); > } > > int > diff --git a/include/systemd-logind.h b/include/systemd-logind.h > index 06dd031..a4067d0 100644 > --- a/include/systemd-logind.h > +++ b/include/systemd-logind.h > @@ -30,14 +30,14 @@ > int systemd_logind_init(void); > void systemd_logind_fini(void); > int systemd_logind_take_fd(int major, int minor, const char *path, Bool > *paus); > -void systemd_logind_release_fd(int major, int minor); > +void systemd_logind_release_fd(int major, int minor, int fd); > int systemd_logind_controls_session(void); > void systemd_logind_vtenter(void); > #else > #define systemd_logind_init() > #define systemd_logind_fini() > #define systemd_logind_take_fd(major, minor, path, paus) -1 > -#define systemd_logind_release_fd(major, minor) > +#define systemd_logind_release_fd(major, minor, fd) close(fd) > #define systemd_logind_controls_session() 0 > #define systemd_logind_vtenter() > #endif > _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
