On Thu, 08 Nov 2012 17:48:44 +0100 Kay Sievers <k...@vrfy.org> wrote:
> On Thu, 2012-11-08 at 11:13 +0100, Robert Milasan wrote:
> > From: Neil Brown <nfbr...@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);
>
>
>
signature.asc
Description: PGP signature
_______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
