[systemd-devel] [PATCH] allow udev to correctly handle 'change' after device has disappeared

[NeilBrown]

On Thu, 08 Nov 2012 17:48:44 +0100 Kay Sievers <kay at vrfy.org> wrote:

> On Thu, 2012-11-08 at 11:13 +0100, Robert Milasan wrote:
> > From: Neil Brown <nfbrown at suse.com>
> > Date: Thu, 8 Nov 2012 10:39:06 +0100
> > Subject: [PATCH] If a 'change' event does not get handled by udev until
> > after the device has subsequently disappeared, udev mis-handles
> >  it. This can happen with 'md' devices which emit a change
> >  event and then a remove event when they are stopped. It is
> >  normally only noticed if udev is very busy (lots of arrays
> >  being stopped at once) or the machine is otherwise loaded
> >  and reponding slowly.
> > 
> > There are two problems.
> > 
> > 1/ udev_device_new_from_syspath() will refuse to create the device
> >   structure if the device does not exist in /sys, and particularly if
> > the uevent file does not exist.
> >   If a 'db' file does exist, that is sufficient evidence that the device
> >   is genuine and should be created.  Equally if we have just received an
> >   event from the kernel about the device, it must be real.
> > 
> >   This patch just disabled the test for the 'uevent' file, it doesn't
> >   try imposing any other tests - it isn't clear that they are really
> >   needed.
> > 
> > 2/ udev_event_execute_rules() calls udev_device_read_db() on a 'device'
> >    structure that is largely uninitialised and in particular does not
> >    have the 'subsystem' set.  udev_device_read_db() needs the subsystem
> >    so it tries to read the 'subsystem' symlink out of sysfs.  If the
> > device is already deleted, this naturally fails.
> >    udev_event_execute_rules() knows the subsystem (as it was in the
> > event message) so this patch simply sets the subsystem for the device
> > structure to be loaded to match the subsystem of the device structure
> > that is handling the event.
> > 
> > With these two changes, deleted handling of change events will still
> > correctly remove any symlinks that are not needed any more.
> 
> The problem is that at the time the 'change' event is handled, the
> device is already removed by the kernel, right? That way we fail to read
> the database with the currently created symlinks?
> 
> The udev_device_new_from_syspath() is a public API from libudev. We
> should probably not change that and not allow other tools than udev
> itself, to create device structures for devices which are not around
> anymore.
> 
> Any chance to check if the patch below fixes the issue you see too. It
> would preserve the current libudev behavior.

Hi Kay,
 yes, that patch looks much nicer than mine.  Testing so far suggests it
 works just as well if not better.

Thanks,
NeilBrown


> 
> Thanks,
> Kay
> 
> diff --git a/src/libudev/libudev-device.c b/src/libudev/libudev-device.c
> index 08476e6..b267141 100644
> --- a/src/libudev/libudev-device.c
> +++ b/src/libudev/libudev-device.c
> @@ -246,7 +246,7 @@ static int udev_device_set_devtype(struct udev_device *udev_device, const char *
>          return 0;
>  }
>  
> -static int udev_device_set_subsystem(struct udev_device *udev_device, const char *subsystem)
> +int udev_device_set_subsystem(struct udev_device *udev_device, const char *subsystem)
>  {
>          free(udev_device->subsystem);
>          udev_device->subsystem = strdup(subsystem);
> diff --git a/src/libudev/libudev-private.h b/src/libudev/libudev-private.h
> index d233565..49cebc1 100644
> --- a/src/libudev/libudev-private.h
> +++ b/src/libudev/libudev-private.h
> @@ -48,6 +48,7 @@ struct udev_list_entry *udev_get_properties_list_entry(struct udev *udev);
>  /* libudev-device.c */
>  struct udev_device *udev_device_new(struct udev *udev);
>  mode_t udev_device_get_devnode_mode(struct udev_device *udev_device);
> +int udev_device_set_subsystem(struct udev_device *udev_device, const char *subsystem);
>  int udev_device_set_syspath(struct udev_device *udev_device, const char *syspath);
>  int udev_device_set_devnode(struct udev_device *udev_device, const char *devnode);
>  int udev_device_add_devlink(struct udev_device *udev_device, const char *devlink);
> diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c
> index 2b9fdf6..39f36f9 100644
> --- a/src/udev/udev-event.c
> +++ b/src/udev/udev-event.c
> @@ -797,8 +797,10 @@ int udev_event_execute_rules(struct udev_event *event, struct udev_rules *rules,
>                  if (major(udev_device_get_devnum(dev)) != 0)
>                          udev_node_remove(dev);
>          } else {
> -                event->dev_db = udev_device_new_from_syspath(event->udev, udev_device_get_syspath(dev));
> +                event->dev_db = udev_device_new(event->udev);
>                  if (event->dev_db != NULL) {
> +                        udev_device_set_syspath(event->dev_db, udev_device_get_syspath(dev));
> +                        udev_device_set_subsystem(event->dev_db, udev_device_get_subsystem(dev));
>                          udev_device_read_db(event->dev_db, NULL);
>                          udev_device_set_info_loaded(event->dev_db);
>  
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 828 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20121119/b84fff20/attachment.pgp>