Merge pull request #8 from Apteryks/udev-monitor-improvements

Udev monitor improvements

NEWS

@@ -43,6 +43,11 @@ Targets are:
 - Document Guile-Udev types.
 
   Thanks to Maxim Cournoyer <[email protected]>.
+** Change the default the timeout values of `make-udev-monitor' to #f
+The TIMEOUT-SEC and TIMEOUT-USEC arguments now default to #f instead of 0,
+which means that the `select' call no longer polls by default, which would
+consume 100% of a CPU core (see:
+https://github.com/artyom-poptsov/guile-udev/issues/5).
 
 * Changes in version 0.2.4 (2022-12-27)
 ** Bugfix: Check every string that comes from the udev for NULL

doc/guile-udev.texi

@@ -182,8 +182,15 @@ Monitor object allows to scan for device events.  When an event occurs, the
 monitor calls a specified callback.  Inside the scanner loop, the monitor
 calls @code{select} procedure to check if any events available.  The
 @code{select} timeout can be set by @code{udev-monitor-set-timeout!}
-procedure.  The timeout seconds and milliseconds parts are set to zero by
-default.
+procedure.  By default, no timeout is used, which means the monitor will wait
+forever for an event to be triggered.
+
+@quotation note
+The monitor scanner loops infinitely; that is, even when a timeout value is
+used, the scanner will keep monitoring events, but will poll at the requested
+period.  For maximum efficiency, it is recommended to not use a timeout and
+let the underlying @code{select} system call indefinitely wait for an event.
+@end quotation
 
 These procedures are available from the module @code{(udev monitor)}.
 
@@ -233,28 +240,34 @@ Example:
 (make-udev-monitor udev #:filter (list "usb" "usb_device"))
 @end lisp
 @item timeout-sec
-Timeout for 'select' call in the event listener.  This part of the timeout is
-specified in seconds.
+Optional timeout for 'select' call in the event listener.  This part of the
+timeout is specified in seconds.
 
 Expected type: Non-negative integer.
 
-Default value: 0
+Default value: @code{#f}
+
+Examples:
+
+@lisp
+(make-udev-monitor udev)   ;no timeout, most efficient
+@end lisp
 
-Example:
 @lisp
-(make-udev-monitor udev #:timeout-sec 1)
+(make-udev-monitor udev #:timeout-sec 3.33)  ;3.33 s poll period
 @end lisp
+
 @item timeout-usec
-Timeout for 'select' call in the event listener.  This part of the timeout is
-specified in milliseconds.
+Optional timeout for 'select' call in the event listener, in microseconds.
+The @var{timeout-sec} argument must be specified as well otherwise
+@var{timeout-usec} is ignored.
 
 Expected type: Non-negative integer.
 
-Default value: 0
+Default value: @code{#f}
 
-Example:
 @lisp
-(make-udev-monitor udev #:timeout-usec 1)
+(make-udev-monitor udev #:timeout-sec 0 #:timeout-usec 250)  ;250 us poll period
 @end lisp
 @end table
 
@@ -278,10 +291,12 @@ Remove all the filters that was set for @var{udev-monitor}. Throws
 @code{guile-udev-error} on errors.
 @end deffn
 
-@deffn {Scheme Procedure} udev-monitor-set-timeout! udev-monitor seconds milliseconds
-Set the @var{udev-monitor} udev event polling timeout. The @var{seconds} and
-@var{milliseconds} parameters must be integer numbers greater or equal to
-zero.
+@deffn {Scheme Procedure} udev-monitor-set-timeout! udev-monitor [secs] [usecs]
+Set the @var{udev-monitor} udev event polling timeout.  The @var{secs}
+parameter must be a non-negative integer or float representing seconds, while
+@var{usecs} must be a non-negative integer representing microseconds.  To
+clear the timeout value, @var{secs} may be set to @code{#f} or omitted
+entirely.
 @end deffn
 
 @deffn {Scheme Procedure} udev-monitor-set-callback! udev-monitor callback

examples/device-listener.scm

@@ -13,11 +13,8 @@
 (define (main args)
   (let* ((udev         (make-udev))
          (udev-monitor (make-udev-monitor udev
-                                          #:timeout-sec  1
-                                          #:timeout-usec 0
-                                          #:callback     callback
-                                          #:filter       (list "usb"
-                                                               "usb_device"))))
+                                          #:callback callback
+                                          #:filter (list "usb" "usb_device"))))
     (udev-monitor-start-scanning! udev-monitor)
     (while #t
       (sleep 1))))

libguile-udev/udev-monitor-func.c

@@ -140,50 +140,56 @@ SCM_DEFINE_N(gudev_monitor_set_error_callback_x, "udev-monitor-set-error-callbac
 }
 #undef FUNC_NAME
 
-SCM_DEFINE_N(gudev_monitor_set_timeout_x, "udev-monitor-set-timeout!", 3,
-             (SCM udev_monitor, SCM seconds, SCM milliseconds),
-             "Set monitor event poll timeout.  \
-Throws 'guile-udev-error' on errors.")
+SCM_DEFINE(gudev_monitor_set_timeout_x, "udev-monitor-set-timeout!", 1, 2, 0,
+	   (SCM udev_monitor, SCM secs, SCM usecs),
+	   "Set monitor event poll @var{seconds} and @var{microseconds}\n"
+	   "timeout non-negative numbers in seconds and microseconds,\n"
+	   "respectively.\n\n"
+
+	   "@var{secs} and @var{usecs} are optional;\n"
+	   "They share the same semantic as the corresponding arguments of\n"
+	   "Guile's `select' abstraction.  When left unspecified, the timeout\n"
+	   "is cleared.\n\n"
+
+	   "Throws 'guile-udev-error' on errors.")
 #define FUNC_NAME s_gudev_monitor_set_timeout_x
 {
      gudev_monitor_t* umd = gudev_monitor_from_scm(udev_monitor);
-     SCM_ASSERT(scm_number_p(seconds),      seconds,      SCM_ARG2, FUNC_NAME);
-     SCM_ASSERT(scm_number_p(milliseconds), milliseconds, SCM_ARG3, FUNC_NAME);
-     long c_seconds      = scm_to_long(seconds);
-     long c_milliseconds = scm_to_long(milliseconds);
-
-     if (c_seconds < 0) {
-          guile_udev_error1(
-               FUNC_NAME,
-               "'Seconds' part of the timeout must be >= 0.",
-               seconds);
-     }
-
-     if (c_milliseconds < 0) {
-          guile_udev_error1(
-               FUNC_NAME,
-               "'Milliseconds' part of the timeout must be >= 0.",
-               milliseconds);
-     }
-
-     umd->timeout.tv_sec  = c_seconds;
-     umd->timeout.tv_usec = c_milliseconds;
+     umd->secs = secs;
+     umd->usecs = usecs;
 
      scm_remember_upto_here_1(udev_monitor);
 
      return SCM_UNDEFINED;
 }
 #undef FUNC_NAME
 
+struct select_args_data {
+     SCM reads;
+     SCM secs;
+     SCM usecs;
+};
+
+/* A procedure for calling scm_select via scm_internal_catch. */
+SCM call_select(void *data) {
+     struct select_args_data* args = data;
+     return scm_select(args->reads, SCM_EOL, SCM_EOL, args->secs, args->usecs);
+}
+
+/* Dummy handler returning false. */
+SCM false_on_exception(void *data, SCM key, SCM args) {
+     return SCM_BOOL_F;
+}
+
 void* udev_monitor_scanner(void* arg)
 #define FUNC_NAME "udev_monitor_scanner"
 {
     SCM udev_monitor = (SCM) arg;
     gudev_monitor_t* umd = gudev_monitor_from_scm(udev_monitor);
-    fd_set fd_set_;
     int result;
-    int select_result;
-    int monitor_fd;
+    SCM select_result;
+    int c_monitor_fd;
+    SCM monitor_fd;
     struct udev_device *dev;
     SCM error_callback = umd->error_callback;
 
@@ -198,38 +204,52 @@ void* udev_monitor_scanner(void* arg)
          return NULL;
     }
 
-    monitor_fd = udev_monitor_get_fd(umd->udev_monitor);
+    c_monitor_fd = udev_monitor_get_fd(umd->udev_monitor);
+    monitor_fd = scm_from_int(c_monitor_fd);
+
     if (monitor_fd < 0) {
-         char msg[] = "Could not udev monitor file descriptor.";
+         char msg[] = "Could not retrieve udev monitor file descriptor.";
          scm_call_2(error_callback, udev_monitor,
                     scm_from_locale_string(msg));
          umd->is_scanning = 0;
          return NULL;
     }
 
+    struct select_args_data select_args;
+    select_args.reads = scm_list_1(monitor_fd);
+    select_args.secs = umd->secs;
+    select_args.usecs = umd->usecs;
+
     SCM callback = umd->scanner_callback;
     SCM device;
+    SCM read_fdes;
 
     while (1) {
-        struct timeval timeout = umd->timeout;
         pthread_mutex_lock(&umd->lock);
         if (! umd->is_scanning) {
             break;
         }
         pthread_mutex_unlock(&umd->lock);
 
-        FD_ZERO(&fd_set_);
-        FD_SET(monitor_fd, &fd_set_);
-        select_result = select(monitor_fd + 1, &fd_set_, NULL, NULL, &timeout);
-        if (select_result == -1) {
+	/* We use scm_select here instead of C select so as to benefit from
+	 * the 'secs' and 'usecs' arguments validation/behavior that it
+	 * provides. */
+	select_result = scm_internal_catch(scm_from_utf8_symbol("system-error"),
+					   call_select, &select_args,
+					   false_on_exception, NULL);
+	// Actual error handling is done here.
+        if (scm_is_false(select_result)) {
              char msg[] = "Error during 'select' call.";
              scm_call_2(error_callback, udev_monitor,
                         scm_from_locale_string(msg));
              umd->is_scanning = 0;
              pthread_cancel(pthread_self());
              break;
         }
-        if (FD_ISSET(monitor_fd, &fd_set_)) {
+
+	read_fdes = scm_car(select_result);
+
+	if (scm_member(monitor_fd, read_fdes)) {
             dev = udev_monitor_receive_device(umd->udev_monitor);
             device = udev_device_to_scm(umd->udev, dev);
             scm_call_1(callback, device);

libguile-udev/udev-monitor-type.c

@@ -81,8 +81,8 @@ SCM gudev_monitor_to_scm(SCM udev, struct udev_monitor *udev_monitor)
     gudev_monitor_t* umd = make_udev_monitor();
     umd->udev             = udev;
     umd->udev_monitor     = udev_monitor;
-    umd->timeout.tv_sec   = 0;
-    umd->timeout.tv_usec  = 0;
+    umd->secs             = SCM_BOOL_F;
+    umd->usecs            = SCM_BOOL_F;
     umd->is_scanning      = 0;
     umd->scanner_callback = SCM_BOOL_F;
     pthread_mutex_init(&umd->lock, NULL);

libguile-udev/udev-monitor-type.h

@@ -45,7 +45,8 @@ struct gudev_monitor {
      /**
       * @brief timeout -- 'select' timeout for the device monitor.
       */
-     struct timeval timeout;
+     SCM secs;
+     SCM usecs;
 
      pthread_mutex_t lock;
      pthread_t scanner_thread;

modules/udev/monitor.scm

@@ -50,18 +50,22 @@
                                               (format (current-error-port)
                                                       "ERROR: in ~a: ~a~%"
                                                       monitor error-message)))
-                            (filter       #f)
-                            (timeout-usec 0)
-                            (timeout-sec  0))
+                            filter
+                            timeout-sec
+                            timeout-usec)
   "Create a new 'udev-monitor' object configured with the specified parameters.
 CALLBACK is a one argument procedure (receiving a 'udev-device' object) called
 when an event matching the specified FILTER.  FILTER is a list whose first
 element is the device subsystem, and whose second argument is the device type,
-or #f to match any type."
+or #f to match any type.  If a timeout is desired, TIMEOUT-SEC or both
+TIMEOUT-SEC and TIMEOUT-USEC may be provided as non-negative numbers of
+seconds and microseconds, respectively.  If TIMEOUT-USEC, is used, TIMEOUT-SEC
+must also have a value, else it is ignored, as for the 'secs' and 'usecs'
+argument of Guile's 'select' procedure."
   (let ((monitor (%make-udev-monitor udev)))
     (udev-monitor-set-timeout!  monitor timeout-sec timeout-usec)
     (udev-monitor-set-callback! monitor callback)
-    (udev-monitor-set-error-callback! monitor callback)
+    (udev-monitor-set-error-callback! monitor error-callback)
     (when filter
       (let ((subsystem (car filter))
             (devtype (cadr filter)))