[PATCH libinput 4/9] evdev: use a udev_device instead of separate sysname/syspath/devnode

Hans de Goede hdegoede at redhat.com
Mon Nov 24 00:55:45 PST 2014


Hi,

On 11/24/2014 01:46 AM, Peter Hutterer wrote:
> Using a udev_device instead of the various bits separately safes us
> re-initializing udev contexts whenever we need to compare the device. And
> having the actual udev device makes it a bit easier to ensure that we're not
> re-initializing a different device as a current one.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>   src/evdev.c     | 85 +++++++++++++++++++++++----------------------------------
>   src/evdev.h     |  9 ++----
>   src/path.c      | 11 ++------
>   src/udev-seat.c | 16 +++++------
>   4 files changed, 46 insertions(+), 75 deletions(-)
>
> diff --git a/src/evdev.c b/src/evdev.c
> index fed66cb..8d457f2 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -240,7 +240,8 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
>   		if (device->mt.slots[slot].seat_slot != -1) {
>   			log_bug_kernel(libinput,
>   				       "%s: Driver sent multiple touch down for the "
> -				       "same slot", device->devnode);
> +				       "same slot",
> +				       udev_device_get_devnode(device->udev_device));
>   			break;
>   		}
>
> @@ -292,7 +293,8 @@ evdev_flush_pending_event(struct evdev_device *device, uint64_t time)
>   		if (device->abs.seat_slot != -1) {
>   			log_bug_kernel(libinput,
>   				       "%s: Driver sent multiple touch down for the "
> -				       "same slot", device->devnode);
> +				       "same slot",
> +				       udev_device_get_devnode(device->udev_device));
>   			break;
>   		}
>
> @@ -1218,20 +1220,11 @@ evdev_need_mtdev(struct evdev_device *device)
>   static void
>   evdev_tag_device(struct evdev_device *device)
>   {
> -	struct udev *udev;
> -	struct udev_device *udev_device = NULL;
> +	struct udev_device *udev_device;
>
> -	udev = udev_new();
> -	if (!udev)
> -		return;
> -
> -	udev_device = udev_device_new_from_syspath(udev, device->syspath);
> -	if (udev_device) {
> -		if (device->dispatch->interface->tag_device)
> -			device->dispatch->interface->tag_device(device, udev_device);
> -		udev_device_unref(udev_device);
> -	}
> -	udev_unref(udev);
> +	udev_device = device->udev_device;
> +	if (device->dispatch->interface->tag_device)
> +		device->dispatch->interface->tag_device(device, udev_device);

Maybe reduce the function to just these 2 lines ?  :

	if (device->dispatch->interface->tag_device)
		device->dispatch->interface->tag_device(device, device->udev_device);

Otherwise looks good:

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

Regards,

Hans

>   }
>
>   static inline int
> @@ -1266,6 +1259,7 @@ evdev_configure_device(struct evdev_device *device)
>   	int active_slot;
>   	int slot;
>   	unsigned int i;
> +	const char *devnode = udev_device_get_devnode(device->udev_device);
>
>   	has_rel = 0;
>   	has_abs = 0;
> @@ -1364,7 +1358,7 @@ evdev_configure_device(struct evdev_device *device)
>   			device->dispatch = evdev_mt_touchpad_create(device);
>   			log_info(libinput,
>   				 "input device '%s', %s is a touchpad\n",
> -				 device->devname, device->devnode);
> +				 device->devname, devnode);
>   			return device->dispatch == NULL ? -1 : 0;
>   		}
>
> @@ -1397,7 +1391,7 @@ evdev_configure_device(struct evdev_device *device)
>
>   		log_info(libinput,
>   			 "input device '%s', %s is a pointer caps =%s%s%s\n",
> -			 device->devname, device->devnode,
> +			 device->devname, devnode,
>   			 has_abs ? " absolute-motion" : "",
>   			 has_rel ? " relative-motion": "",
>   			 has_button ? " button" : "");
> @@ -1417,13 +1411,13 @@ evdev_configure_device(struct evdev_device *device)
>   		device->seat_caps |= EVDEV_DEVICE_KEYBOARD;
>   		log_info(libinput,
>   			 "input device '%s', %s is a keyboard\n",
> -			 device->devname, device->devnode);
> +			 device->devname, devnode);
>   	}
>   	if (has_touch && !has_button) {
>   		device->seat_caps |= EVDEV_DEVICE_TOUCH;
>   		log_info(libinput,
>   			 "input device '%s', %s is a touch device\n",
> -			 device->devname, device->devnode);
> +			 device->devname, devnode);
>   	}
>
>   	return 0;
> @@ -1457,15 +1451,14 @@ evdev_notify_added_device(struct evdev_device *device)
>
>   struct evdev_device *
>   evdev_device_create(struct libinput_seat *seat,
> -		    const char *devnode,
> -		    const char *sysname,
> -		    const char *syspath)
> +		    struct udev_device *udev_device)
>   {
>   	struct libinput *libinput = seat->libinput;
>   	struct evdev_device *device = NULL;
>   	int rc;
>   	int fd;
>   	int unhandled_device = 0;
> +	const char *devnode = udev_device_get_devnode(udev_device);
>
>   	/* Use non-blocking mode so that we can loop on read on
>   	 * evdev_device_data() until all events on the fd are
> @@ -1494,9 +1487,7 @@ evdev_device_create(struct libinput_seat *seat,
>   	device->seat_caps = 0;
>   	device->is_mt = 0;
>   	device->mtdev = NULL;
> -	device->devnode = strdup(devnode);
> -	device->sysname = strdup(sysname);
> -	device->syspath = strdup(syspath);
> +	device->udev_device = udev_device_ref(udev_device);
>   	device->rel.dx = 0;
>   	device->rel.dy = 0;
>   	device->abs.seat_slot = -1;
> @@ -1565,7 +1556,7 @@ evdev_device_get_output(struct evdev_device *device)
>   const char *
>   evdev_device_get_sysname(struct evdev_device *device)
>   {
> -	return device->sysname;
> +	return udev_device_get_sysname(device->udev_device);
>   }
>
>   const char *
> @@ -1919,32 +1910,25 @@ evdev_device_suspend(struct evdev_device *device)
>   }
>
>   static int
> -evdev_device_compare_syspath(struct evdev_device *device, int fd)
> +evdev_device_compare_syspath(struct udev_device *udev_device, int fd)
>   {
> -	struct udev *udev = NULL;
> -	struct udev_device *udev_device = NULL;
> -	const char *syspath;
> +	struct udev *udev = udev_device_get_udev(udev_device);
> +	struct udev_device *udev_device_new;
>   	struct stat st;
>   	int rc = 1;
>
> -	udev = udev_new();
> -	if (!udev)
> -		goto out;
> -
>   	if (fstat(fd, &st) < 0)
>   		goto out;
>
> -	udev_device = udev_device_new_from_devnum(udev, 'c', st.st_rdev);
> -	if (!device)
> +	udev_device_new = udev_device_new_from_devnum(udev, 'c', st.st_rdev);
> +	if (!udev_device_new)
>   		goto out;
>
> -	syspath = udev_device_get_syspath(udev_device);
> -	rc = strcmp(syspath, device->syspath);
> +	rc = strcmp(udev_device_get_syspath(udev_device_new),
> +		    udev_device_get_syspath(udev_device));
>   out:
> -	if (udev_device)
> -		udev_device_unref(udev_device);
> -	if (udev)
> -		udev_unref(udev);
> +	if (udev_device_new)
> +		udev_device_unref(udev_device_new);
>   	return rc;
>   }
>
> @@ -1953,19 +1937,21 @@ evdev_device_resume(struct evdev_device *device)
>   {
>   	struct libinput *libinput = device->base.seat->libinput;
>   	int fd;
> +	const char *devnode;
>
>   	if (device->fd != -1)
>   		return 0;
>
> -	if (device->syspath == NULL)
> +	if (device->was_removed)
>   		return -ENODEV;
>
> -	fd = open_restricted(libinput, device->devnode, O_RDWR | O_NONBLOCK);
> +	devnode = udev_device_get_devnode(device->udev_device);
> +	fd = open_restricted(libinput, devnode, O_RDWR | O_NONBLOCK);
>
>   	if (fd < 0)
>   		return -errno;
>
> -	if (evdev_device_compare_syspath(device, fd)) {
> +	if (evdev_device_compare_syspath(device->udev_device, fd)) {
>   		close_restricted(libinput, fd);
>   		return -ENODEV;
>   	}
> @@ -2008,10 +1994,9 @@ evdev_device_remove(struct evdev_device *device)
>
>   	evdev_device_suspend(device);
>
> -	/* A device may be removed while suspended. Free the syspath to
> +	/* A device may be removed while suspended, mark it to
>   	 * skip re-opening a different device with the same node */
> -	free(device->syspath);
> -	device->syspath = NULL;
> +	device->was_removed = true;
>
>   	list_remove(&device->base.link);
>
> @@ -2031,9 +2016,7 @@ evdev_device_destroy(struct evdev_device *device)
>   	filter_destroy(device->pointer.filter);
>   	libinput_seat_unref(device->base.seat);
>   	libevdev_free(device->evdev);
> +	udev_device_unref(device->udev_device);
>   	free(device->mt.slots);
> -	free(device->devnode);
> -	free(device->sysname);
> -	free(device->syspath);
>   	free(device);
>   }
> diff --git a/src/evdev.h b/src/evdev.h
> index 35da319..0e8b445 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -68,11 +68,10 @@ struct evdev_device {
>
>   	struct evdev_dispatch *dispatch;
>   	struct libevdev *evdev;
> +	struct udev_device *udev_device;
>   	char *output_name;
> -	char *devnode;
> -	char *sysname;
> -	char *syspath;
>   	const char *devname;
> +	bool was_removed;
>   	int fd;
>   	struct {
>   		const struct input_absinfo *absinfo_x, *absinfo_y;
> @@ -203,9 +202,7 @@ struct evdev_dispatch {
>
>   struct evdev_device *
>   evdev_device_create(struct libinput_seat *seat,
> -		    const char *devnode,
> -		    const char *sysname,
> -		    const char *syspath);
> +		    struct udev_device *device);
>
>   int
>   evdev_device_init_pointer_acceleration(struct evdev_device *device);
> diff --git a/src/path.c b/src/path.c
> index 175366b..9d0632a 100644
> --- a/src/path.c
> +++ b/src/path.c
> @@ -115,14 +115,10 @@ path_device_enable(struct path_input *input,
>   {
>   	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;
>
> -	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);
>
> @@ -144,7 +140,7 @@ path_device_enable(struct path_input *input,
>   		}
>   	}
>
> -	device = evdev_device_create(&seat->base, devnode, sysname, syspath);
> +	device = evdev_device_create(&seat->base, udev_device);
>   	libinput_seat_unref(&seat->base);
>
>   	if (device == EVDEV_UNHANDLED_DEVICE) {
> @@ -161,8 +157,6 @@ path_device_enable(struct path_input *input,
>   	}
>
>   out:
> -	free(sysname);
> -	free(syspath);
>   	free(seat_name);
>   	free(seat_logical_name);
>
> @@ -313,8 +307,7 @@ libinput_path_remove_device(struct libinput_device *device)
>   	}
>
>   	list_for_each(dev, &input->path_list, link) {
> -		const char *devnode = udev_device_get_devnode(dev->udev_device);
> -		if (strcmp(evdev->devnode, devnode) == 0) {
> +		if (dev->udev_device == evdev->udev_device) {
>   			list_remove(&dev->link);
>   			udev_device_unref(dev->udev_device);
>   			free(dev);
> diff --git a/src/udev-seat.c b/src/udev-seat.c
> index f2be66e..49c8f47 100644
> --- a/src/udev-seat.c
> +++ b/src/udev-seat.c
> @@ -46,8 +46,6 @@ device_added(struct udev_device *udev_device, struct udev_input *input)
>   {
>   	struct evdev_device *device;
>   	const char *devnode;
> -	const char *sysname;
> -	const char *syspath;
>   	const char *device_seat, *seat_name, *output_name;
>   	const char *calibration_values;
>   	float calibration[6];
> @@ -61,8 +59,6 @@ device_added(struct udev_device *udev_device, struct udev_input *input)
>   		return 0;
>
>   	devnode = udev_device_get_devnode(udev_device);
> -	sysname = udev_device_get_sysname(udev_device);
> -	syspath = udev_device_get_syspath(udev_device);
>
>   	/* Search for matching logical seat */
>   	seat_name = udev_device_get_property_value(udev_device, "WL_SEAT");
> @@ -79,7 +75,7 @@ device_added(struct udev_device *udev_device, struct udev_input *input)
>   			return -1;
>   	}
>
> -	device = evdev_device_create(&seat->base, devnode, sysname, syspath);
> +	device = evdev_device_create(&seat->base, udev_device);
>   	libinput_seat_unref(&seat->base);
>
>   	if (device == EVDEV_UNHANDLED_DEVICE) {
> @@ -123,18 +119,20 @@ device_added(struct udev_device *udev_device, struct udev_input *input)
>   static void
>   device_removed(struct udev_device *udev_device, struct udev_input *input)
>   {
> -	const char *devnode;
>   	struct evdev_device *device, *next;
>   	struct udev_seat *seat;
> +	const char *syspath;
>
> -	devnode = udev_device_get_devnode(udev_device);
> +	syspath = udev_device_get_syspath(udev_device);
>   	list_for_each(seat, &input->base.seat_list, base.link) {
>   		list_for_each_safe(device, next,
>   				   &seat->base.devices_list, base.link) {
> -			if (!strcmp(device->devnode, devnode)) {
> +			if (!strcmp(syspath,
> +				    udev_device_get_syspath(device->udev_device))) {
>   				log_info(&input->base,
>   					 "input device %s, %s removed\n",
> -					 device->devname, device->devnode);
> +					 device->devname,
> +					 udev_device_get_devnode(device->udev_device));
>   				evdev_device_remove(device);
>   				break;
>   			}
>


More information about the wayland-devel mailing list