[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