[PATCH] systemd-logind: let the logind code decided whether to close an fd
Hans de Goede
hdegoede at redhat.com
Fri May 2 02:16:13 PDT 2014
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 <peter.hutterer at who-t.net>
Thanks for catching this, looks good. One minor nitpick below. With that
nitpick fixed this is:
Reviewed-by: Hans de Goede <hdegoede at redhat.com>
> ---
> 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
>
More information about the xorg-devel
mailing list