On 9/7/09, Florian Zumbiehl <florz@xxxxxxxx> wrote:
> Hi,
>
>> > int util_create_path(struct udev *udev, const char *path)
>> > {
>> > char p[UTIL_PATH_SIZE];
>> > @@ -32,6 +55,8 @@ int util_create_path(struct udev *udev, const char
>> > *path)
>> > struct stat stats;
>> > int ret;
>> >
>> > + if (util_path_lock())
>> > + return -1;
>>
>> I think the patch missed some call sites. Each create_path() needs to
>> be followed by a create_path_done(), but I can see nine calls to
>> create_path() calls and only about six to create_path_done().
>
> Hu? I see 8 call sites, and those should all be covered - maybe it's just
> because I'm working on a somewhat old tree?
The ninth site was the recursive call within create_path(). I see
you've added unlock()s there now. I re-counted on a per-file basis
and everything else looks covered; I probably miscounted before.
I'm still concerned/interested about the effect on performance. But
since I read the patch, here are some style suggestions.
> [...]
>> I think there's locking recursion here, because we end up calling
>> name_index() between create_path() and create_path_done(). Looking
>> at the definition of unlock(), that's not going to work.
>
> Below is a new patch that should work with recursive locking (of limited
> depth - I hope 2^31-1 levels of recursion is sufficient? :-)
>
> Untested, uncompiled, etc., as usual ...
>
> Florian
>
> diff --git a/libudev/libudev-device-private.c
> b/libudev/libudev-device-private.c
> index 68dc0a5..d386135 100644
> --- a/libudev/libudev-device-private.c
> +++ b/libudev/libudev-device-private.c
> @@ -44,7 +44,8 @@ int udev_device_update_db(struct udev_device *udev_device)
> int ret;
>
> devpath_to_db_path(udev, udev_device_get_devpath(udev_device), filename,
> sizeof(filename));
> - util_create_path(udev, filename);
> + if (util_create_path(udev, filename))
> + return -1;
> unlink(filename);
>
> udev_list_entry_foreach(list_entry,
> udev_device_get_properties_list_entry(udev_device))
> @@ -86,6 +87,7 @@ file:
> f = fopen(filename, "w");
> if (f == NULL) {
> err(udev, "unable to create db file '%s': %m\n", filename);
> + util_create_path_done();
> return -1;
> }
> info(udev, "created db file for '%s' in '%s'\n",
> udev_device_get_devpath(udev_device), filename);
> @@ -114,6 +116,7 @@ file:
> }
> fclose(f);
> out:
> + util_create_path_done();
> return 0;
> }
>
> diff --git a/libudev/libudev-queue-private.c
> b/libudev/libudev-queue-private.c
> index 0427b65..f2f3905 100644
> --- a/libudev/libudev-queue-private.c
> +++ b/libudev/libudev-queue-private.c
> @@ -417,6 +417,7 @@ static void update_failed(struct udev_queue_export
> *udev_queue_export,
> udev_selinux_setfscreatecon(udev, filename, S_IFLNK);
> symlink(udev_device_get_devpath(udev_device), filename);
> udev_selinux_resetfscreatecon(udev);
> + util_create_path_done();
> break;
>
> case DEVICE_QUEUED:
> diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
> index c309945..26fbb5c 100644
> --- a/libudev/libudev-util-private.c
> +++ b/libudev/libudev-util-private.c
> @@ -25,6 +25,28 @@
> #include "libudev.h"
> #include "libudev-private.h"
>
> +static int util_path_lock_cnt,util_path_lock_fd;
static int util_path_lock_cnt;
static int util_path_lock_fd;
> +static int util_path_lock(void)
> +{
> + if (!util_path_lock_cnt) {
> + if ((util_path_lock_fd = open("/dev/udev_util_path_lock", O_WRONLY |
> O_CREAT)) < 0)
> + return -1;
> + if (flock(util_path_lock_fd, LOCK_EX)) {
> + close(util_create_path_lock_fd);
> + return -1;
> + }
> + util_path_lock_cnt++;
> + }
> + return 0;
> +}
> +
> +static void util_path_unlock(void)
> +{
> + if (!--util_path_lock_cnt)
> + close(util_create_path_lock_fd);
> +}
> +
> int util_create_path(struct udev *udev, const char *path)
> {
> char p[UTIL_PATH_SIZE];
> @@ -32,6 +54,8 @@ int util_create_path(struct udev *udev, const char *path)
> struct stat stats;
> int ret;
>
> + if (util_path_lock())
> + return -1;
>
> util_strscpy(p, sizeof(p), path);
> pos = strrchr(p, '/');
> if (pos == NULL)
> @@ -47,8 +71,10 @@ int util_create_path(struct udev *udev, const char *path)
> if (stat(p, &stats) == 0 && (stats.st_mode & S_IFMT) == S_IFDIR)
> return 0;
>
> - if (util_create_path(udev, p) != 0)
> + if (util_create_path(udev, p) != 0) {
> + util_path_unlock();
> return -1;
> + }
Looks suspicious. It'll try to unlock even If the recursive
util_create_path() bailed because it failed at util_path_lock().
I suspect that can't happen, but this seems needlessly tricky. How
about just renaming the old create_path() to create_path_unlocked() or
something (updating the recursive call accordingly), and then
int util_create_path(struct udev *udev, const char *path)
{
if (!util_path_lock())
return -1;
return util_path_create_unlocked(udev, path);
}
> dbg(udev, "mkdir '%s'\n", p);
> udev_selinux_setfscreatecon(udev, p, S_IFDIR|0755);
> @@ -60,9 +86,15 @@ int util_create_path(struct udev *udev, const char *path)
> if (ret == 0)
> return 0;
>
> + util_path_unlock();
> return -1;
> }
>
> +void util_create_path_done(void)
> +{
> + util_path_unlock();
> +}
> +
> int util_delete_path(struct udev *udev, const char *path)
> {
> char p[UTIL_PATH_SIZE];
> @@ -75,6 +107,8 @@ int util_delete_path(struct udev *udev, const char *path)
> if (pos == p || pos == NULL)
> return 0;
>
> + if (util_path_lock())
> + return 0;
> while (1) {
> *pos = '\0';
> pos = strrchr(p, '/');
> @@ -88,13 +122,13 @@ int util_delete_path(struct udev *udev, const char
> *path)
> if (errno == ENOENT)
> retval = 0;
> if (retval) {
> - if (errno == ENOTEMPTY)
> - return 0;
> - err(udev, "rmdir(%s) failed: %m\n", p);
> + if (errno != ENOTEMPTY)
> + err(udev, "rmdir(%s) failed: %m\n", p);
> break;
> }
> dbg(udev, "removed '%s'\n", p);
> }
> + util_path_unlock();
> return 0;
> }
Hmm, does this function ever return non-zero? Is it time to admit
that it returns void?
Regards
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
