[PATCH libinput 3/9] path: store the udev device instead of just the devnode

Hans de Goede hdegoede at redhat.com
Mon Nov 24 00:51:44 PST 2014


Hi,

On 11/24/2014 01:46 AM, Peter Hutterer wrote:
> Long-term plan to use more of udev_device here is to better protect us against
> re-opening a different device that happens to have the same devnode.
>
> This now also prints an error message for invalid devices, the log tests are
> adjusted.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>   src/path.c | 109 ++++++++++++++++++++++++++-----------------------------------
>   src/path.h |   2 +-
>   test/log.c |   8 +++--
>   3 files changed, 53 insertions(+), 66 deletions(-)
>
> diff --git a/src/path.c b/src/path.c
> index 5900775..175366b 100644
> --- a/src/path.c
> +++ b/src/path.c
> @@ -109,62 +109,26 @@ path_seat_get_named(struct path_input *input,
>   	return NULL;
>   }
>
> -static int
> -path_get_udev_properties(struct udev *udev,
> -			 const char *path,
> -			 char **sysname,
> -			 char **syspath,
> -			 char **seat_name,
> -			 char **seat_logical_name)
> -{
> -	struct udev_device *device = NULL;
> -	struct stat st;
> -	const char *seat;
> -	int rc = -1;
> -
> -	if (stat(path, &st) < 0)
> -		goto out;
> -
> -	device = udev_device_new_from_devnum(udev, 'c', st.st_rdev);
> -	if (!device)
> -		goto out;
> -
> -	*sysname = strdup(udev_device_get_sysname(device));
> -	*syspath = strdup(udev_device_get_syspath(device));
> -
> -	seat = udev_device_get_property_value(device, "ID_SEAT");
> -	*seat_name = strdup(seat ? seat : default_seat);
> -
> -	seat = udev_device_get_property_value(device, "WL_SEAT");
> -	*seat_logical_name = strdup(seat ? seat : default_seat_name);
> -
> -	rc = 0;
> -
> -out:
> -	if (device)
> -		udev_device_unref(device);
> -	return rc;
> -}
> -
>   static struct libinput_device *
> -path_device_enable(struct path_input *input, const char *devnode)
> +path_device_enable(struct path_input *input,
> +		   struct udev_device *udev_device)
>   {
>   	struct path_seat *seat;
>   	struct evdev_device *device = NULL;
>   	char *sysname = NULL, *syspath = NULL;
>   	char *seat_name = NULL, *seat_logical_name = NULL;
> +	const char *seat_prop;
> +	const char *devnode;
>
> -	if (path_get_udev_properties(input->udev,
> -				     devnode,
> -				     &sysname,
> -				     &syspath,
> -				     &seat_name,
> -				     &seat_logical_name) == -1) {
> -		log_info(&input->base,
> -			 "failed to obtain sysname for device '%s'.\n",
> -			 devnode);
> -		return NULL;
> -	}
> +	sysname = strdup(udev_device_get_sysname(udev_device));
> +	syspath = strdup(udev_device_get_syspath(udev_device));
> +
> +	seat_prop = udev_device_get_property_value(udev_device, "ID_SEAT");
> +	seat_name = strdup(seat_prop ? seat_prop : default_seat);
> +
> +	seat_prop = udev_device_get_property_value(udev_device, "WL_SEAT");
> +	seat_logical_name = strdup(seat_prop ? seat_prop : default_seat_name);
> +	devnode = udev_device_get_devnode(udev_device);
>
>   	seat = path_seat_get_named(input, seat_name, seat_logical_name);
>
> @@ -212,7 +176,7 @@ path_input_enable(struct libinput *libinput)
>   	struct path_device *dev;
>
>   	list_for_each(dev, &input->path_list, link) {
> -		if (path_device_enable(input, dev->path) == NULL) {
> +		if (path_device_enable(input, dev->udev_device) == NULL) {
>   			path_input_disable(libinput);
>   			return -1;
>   		}
> @@ -230,7 +194,7 @@ path_input_destroy(struct libinput *input)
>   	udev_unref(path_input->udev);
>
>   	list_for_each_safe(dev, tmp, &path_input->path_list, link) {
> -		free(dev->path);
> +		udev_device_unref(dev->udev_device);
>   		free(dev);
>   	}
>
> @@ -238,7 +202,7 @@ path_input_destroy(struct libinput *input)
>
>   static struct libinput_device *
>   path_create_device(struct libinput *libinput,
> -		   const char *path)
> +		   struct udev_device *udev_device)
>   {
>   	struct path_input *input = (struct path_input*)libinput;
>   	struct path_device *dev;
> @@ -248,19 +212,15 @@ path_create_device(struct libinput *libinput,
>   	if (!dev)
>   		return NULL;
>
> -	dev->path = strdup(path);
> -	if (!dev->path) {
> -		free(dev);
> -		return NULL;
> -	}
> +	dev->udev_device = udev_device_ref(udev_device);
>
>   	list_insert(&input->path_list, &dev->link);
>
> -	device = path_device_enable(input, dev->path);
> +	device = path_device_enable(input, udev_device);
>
>   	if (!device) {
> +		udev_device_unref(udev_device);

Nitpick, it would be better to use: udev_device_unref(dev->udev_device);
(same thing, but clearer signals intent).

Otherwise looks good:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Regards,

Hans



>   		list_remove(&dev->link);
> -		free(dev->path);
>   		free(dev);
>   	}
>
> @@ -302,16 +262,40 @@ libinput_path_create_context(const struct libinput_interface *interface,
>   	return &input->base;
>   }
>
> +static inline struct udev_device *
> +udev_device_from_devnode(struct udev *udev, const char *devnode)
> +{
> +	struct stat st;
> +
> +	if (stat(devnode, &st) < 0)
> +		return NULL;
> +
> +	return udev_device_new_from_devnum(udev, 'c', st.st_rdev);
> +}
> +
>   LIBINPUT_EXPORT struct libinput_device *
>   libinput_path_add_device(struct libinput *libinput,
>   			 const char *path)
>   {
> +	struct path_input *input = (struct path_input *)libinput;
> +	struct udev *udev = input->udev;
> +	struct udev_device *udev_device;
> +	struct libinput_device *device;
> +
>   	if (libinput->interface_backend != &interface_backend) {
>   		log_bug_client(libinput, "Mismatching backends.\n");
>   		return NULL;
>   	}
>
> -	return path_create_device(libinput, path);
> +	udev_device = udev_device_from_devnode(udev, path);
> +	if (!udev_device) {
> +		log_bug_client(libinput, "Invalid path %s\n", path);
> +		return NULL;
> +	}
> +
> +	device = path_create_device(libinput, udev_device);
> +	udev_device_unref(udev_device);
> +	return device;
>   }
>
>   LIBINPUT_EXPORT void
> @@ -329,9 +313,10 @@ libinput_path_remove_device(struct libinput_device *device)
>   	}
>
>   	list_for_each(dev, &input->path_list, link) {
> -		if (strcmp(evdev->devnode, dev->path) == 0) {
> +		const char *devnode = udev_device_get_devnode(dev->udev_device);
> +		if (strcmp(evdev->devnode, devnode) == 0) {
>   			list_remove(&dev->link);
> -			free(dev->path);
> +			udev_device_unref(dev->udev_device);
>   			free(dev);
>   			break;
>   		}
> diff --git a/src/path.h b/src/path.h
> index 999601b..973146a 100644
> --- a/src/path.h
> +++ b/src/path.h
> @@ -34,7 +34,7 @@ struct path_input {
>
>   struct path_device {
>   	struct list link;
> -	char *path;
> +	struct udev_device *udev_device;
>   };
>
>   struct path_seat {
> diff --git a/test/log.c b/test/log.c
> index 6ce5e7f..a56af15 100644
> --- a/test/log.c
> +++ b/test/log.c
> @@ -125,11 +125,13 @@ START_TEST(log_priority)
>
>   	libinput_path_add_device(li, "/tmp");
>
> -	ck_assert_int_eq(log_handler_called, 0);
> +	ck_assert_int_eq(log_handler_called, 1);
>
>   	libinput_log_set_priority(li, LIBINPUT_LOG_PRIORITY_INFO);
> -	libinput_path_add_device(li, "/tmp");
> -	ck_assert_int_gt(log_handler_called, 0);
> +	/* event0 is usually Lid Switch which prints an info that
> +	   we don't handle it */
> +	libinput_path_add_device(li, "/dev/input/event0");
> +	ck_assert_int_gt(log_handler_called, 1);
>
>   	log_handler_called = 0;
>
>


More information about the wayland-devel mailing list