Re: [PATCH v2 libinput 2/4] Move opening and closing the device fd into evdev.c
On Thu, Feb 06, 2014 at 09:20:15AM +1000, Peter Hutterer wrote:
> evdev_device_remove() already calls close(device->fd). Move the
> close_restricted call there to avoid one privileged call in the backend and
> one in the device. And move the open_restricted() into the evdev device too to
> reduce the duplicated code in the two backends.
>
> Update to one of the tests: since we'd now fail getting the device node from
> the invalid /tmp path, the open_func_count is 0.
This good I think.
Jonas
>
> Signed-off-by: Peter Hutterer
> ---
> Changes to v1:
> - move open_restricted into evdev_device_create()
>
> If we really get an fd open failure, we now get two error messages, but I'll
> fix that up in a follow-up restructure.
>
> src/evdev.c | 18 +++---
> src/evdev.h | 3 +--
> src/path.c | 14 +-
> src/udev-seat.c | 20 +---
> test/path.c | 2 +-
> 5 files changed, 19 insertions(+), 38 deletions(-)
>
> diff --git a/src/evdev.c b/src/evdev.c
> index 2bc301b..61ab083 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -600,12 +600,22 @@ evdev_configure_device(struct evdev_device *device)
> struct evdev_device *
> evdev_device_create(struct libinput_seat *seat,
> const char *devnode,
> - const char *sysname,
> - int fd)
> + const char *sysname)
> {
> struct libinput *libinput = seat->libinput;
> struct evdev_device *device;
> char devname[256] = "unknown";
> + int fd;
> +
> + /* Use non-blocking mode so that we can loop on read on
> + * evdev_device_data() until all events on the fd are
> + * read. mtdev_get() also expects this. */
> + fd = open_restricted(libinput, devnode, O_RDWR | O_NONBLOCK);
> + if (fd < 0) {
> + log_info("opening input device '%s' failed (%s).\n",
> + devnode, strerror(-fd));
> + return NULL;
> + }
>
> device = zalloc(sizeof *device);
> if (device == NULL)
> @@ -655,6 +665,8 @@ evdev_device_create(struct libinput_seat *seat,
> return device;
>
> err:
> + if (fd >= 0)
> + close_restricted(libinput, fd);
> evdev_device_destroy(device);
> return NULL;
> }
> @@ -710,7 +722,7 @@ evdev_device_remove(struct evdev_device *device)
>
> if (device->mtdev)
> mtdev_close_delete(device->mtdev);
> - close(device->fd);
> + close_restricted(device->base.seat->libinput, device->fd);
> list_remove(&device->base.link);
>
> notify_removed_device(&device->base);
> diff --git a/src/evdev.h b/src/evdev.h
> index 37c32e5..3c9f93a 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -118,8 +118,7 @@ struct evdev_dispatch {
> struct evdev_device *
> evdev_device_create(struct libinput_seat *seat,
> const char *devnode,
> - const char *sysname,
> - int fd);
> + const char *sysname);
>
> struct evdev_dispatch *
> evdev_touchpad_create(struct evdev_device *device);
> diff --git a/src/path.c b/src/path.c
> index 2893ad4..2254bbe 100644
> --- a/src/path.c
> +++ b/src/path.c
> @@ -42,7 +42,6 @@ path_input_disable(struct libinput *libinput)
> struct evdev_device *device = input->device;
>
> if (device) {
> - close_restricted(libinput, device->fd);
> evdev_device_remove(device);
> input->device = NULL;
> }
> @@ -122,22 +121,13 @@ path_input_enable(struct libinput *libinput)
> struct evdev_device *device;
> const char *devnode = input->path;
> char *sysname;
> - int fd;
> char *seat_name, *seat_logical_name;
>
> if (input->device)
> return 0;
>
> - fd = open_restricted(libinput, devnode, O_RDWR|O_NONBLOCK);
> - if (fd < 0) {
> - log_info("opening input device '%s' failed (%s).\n",
> - devnode, strerror(-fd));
> - return -1;
> - }
> -
> if (path_get_udev_properties(devnode, &sysname,
>&seat_name, &seat_logical_name) == -1) {
> - close_restricted(libinput, fd);
> log_info("failed to obtain sysname for device '%s'.\n",
> devnode);
> return -1;
> }
> @@ -146,16 +136,14 @@ path_input_enable(struct libinput *libinput)
> free(seat_name);
> free(seat_logical_name);
>
> - device = evdev_device_create(&seat->base, devnode, sysname, fd);
> + device = evdev_device_create(&seat->base, devnode, sysname);
> free(sysname);
> libinput_seat_unref(&seat->base);
>
> if (device == EVDEV_UNHANDLED_DEVICE) {
> - close_restricted(libinput, fd);
> log_info("not using input device '%s'.\n", devnode);
> return -1;
> } else if (device == NULL) {
> - close_restricted(libinput, fd);
> log_info("failed
[PATCH v2 libinput 2/4] Move opening and closing the device fd into evdev.c
evdev_device_remove() already calls close(device->fd). Move the
close_restricted call there to avoid one privileged call in the backend and
one in the device. And move the open_restricted() into the evdev device too to
reduce the duplicated code in the two backends.
Update to one of the tests: since we'd now fail getting the device node from
the invalid /tmp path, the open_func_count is 0.
Signed-off-by: Peter Hutterer
---
Changes to v1:
- move open_restricted into evdev_device_create()
If we really get an fd open failure, we now get two error messages, but I'll
fix that up in a follow-up restructure.
src/evdev.c | 18 +++---
src/evdev.h | 3 +--
src/path.c | 14 +-
src/udev-seat.c | 20 +---
test/path.c | 2 +-
5 files changed, 19 insertions(+), 38 deletions(-)
diff --git a/src/evdev.c b/src/evdev.c
index 2bc301b..61ab083 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -600,12 +600,22 @@ evdev_configure_device(struct evdev_device *device)
struct evdev_device *
evdev_device_create(struct libinput_seat *seat,
const char *devnode,
- const char *sysname,
- int fd)
+ const char *sysname)
{
struct libinput *libinput = seat->libinput;
struct evdev_device *device;
char devname[256] = "unknown";
+ int fd;
+
+ /* Use non-blocking mode so that we can loop on read on
+* evdev_device_data() until all events on the fd are
+* read. mtdev_get() also expects this. */
+ fd = open_restricted(libinput, devnode, O_RDWR | O_NONBLOCK);
+ if (fd < 0) {
+ log_info("opening input device '%s' failed (%s).\n",
+devnode, strerror(-fd));
+ return NULL;
+ }
device = zalloc(sizeof *device);
if (device == NULL)
@@ -655,6 +665,8 @@ evdev_device_create(struct libinput_seat *seat,
return device;
err:
+ if (fd >= 0)
+ close_restricted(libinput, fd);
evdev_device_destroy(device);
return NULL;
}
@@ -710,7 +722,7 @@ evdev_device_remove(struct evdev_device *device)
if (device->mtdev)
mtdev_close_delete(device->mtdev);
- close(device->fd);
+ close_restricted(device->base.seat->libinput, device->fd);
list_remove(&device->base.link);
notify_removed_device(&device->base);
diff --git a/src/evdev.h b/src/evdev.h
index 37c32e5..3c9f93a 100644
--- a/src/evdev.h
+++ b/src/evdev.h
@@ -118,8 +118,7 @@ struct evdev_dispatch {
struct evdev_device *
evdev_device_create(struct libinput_seat *seat,
const char *devnode,
- const char *sysname,
- int fd);
+ const char *sysname);
struct evdev_dispatch *
evdev_touchpad_create(struct evdev_device *device);
diff --git a/src/path.c b/src/path.c
index 2893ad4..2254bbe 100644
--- a/src/path.c
+++ b/src/path.c
@@ -42,7 +42,6 @@ path_input_disable(struct libinput *libinput)
struct evdev_device *device = input->device;
if (device) {
- close_restricted(libinput, device->fd);
evdev_device_remove(device);
input->device = NULL;
}
@@ -122,22 +121,13 @@ path_input_enable(struct libinput *libinput)
struct evdev_device *device;
const char *devnode = input->path;
char *sysname;
- int fd;
char *seat_name, *seat_logical_name;
if (input->device)
return 0;
- fd = open_restricted(libinput, devnode, O_RDWR|O_NONBLOCK);
- if (fd < 0) {
- log_info("opening input device '%s' failed (%s).\n",
-devnode, strerror(-fd));
- return -1;
- }
-
if (path_get_udev_properties(devnode, &sysname,
&seat_name, &seat_logical_name) == -1) {
- close_restricted(libinput, fd);
log_info("failed to obtain sysname for device '%s'.\n",
devnode);
return -1;
}
@@ -146,16 +136,14 @@ path_input_enable(struct libinput *libinput)
free(seat_name);
free(seat_logical_name);
- device = evdev_device_create(&seat->base, devnode, sysname, fd);
+ device = evdev_device_create(&seat->base, devnode, sysname);
free(sysname);
libinput_seat_unref(&seat->base);
if (device == EVDEV_UNHANDLED_DEVICE) {
- close_restricted(libinput, fd);
log_info("not using input device '%s'.\n", devnode);
return -1;
} else if (device == NULL) {
- close_restricted(libinput, fd);
log_info("failed to create input device '%s'.\n", devnode);
return -1;
}
diff --git a/src/udev-seat.c b/src/udev-seat.c
index 5936511..957e762 100644
--- a/src/udev-seat.c
+++ b/src/udev-seat.c
@@ -44,13 +44,11 @
