On 9/4/09, Florian Zumbiehl <florz@xxxxxxxx> wrote:
> Hi,
>
>> That would limit paralellism to some degree.
>>
>> Retrys won't be needed very often; they would surely have a lower
>> overhead. Less code too :-). It's not completely out of left field;
>> both seqlocks and transactional memory use retries. There's no risk
>> of livelock (i.e. infinite retries)...
>>
>> I'm lazy and don't want udev to run measurably slower. The only
>> advantage I see of flock() is that it tells the reader "I'm a
>> synchronization primitive, solving a concurrency problem here". I
>> think we can get close enough by explaining the race in a comment next
>> to the definition of create_path().
>
> plus, it seems to be the simpler solution by far - which is why I went
> with flock() in my suggested fix you find below. Untested, as usual, and
> I don't really have a clue whether there are any unhealthy interactions
> with the rest of udev in this solution. Also, you most likely will want
> to change the path of the lock file ;-)
>
> Florian
> 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().
>diff --git a/udev/udev-node.c b/udev/udev-node.c
> index d73bc6c..74de922 100644
> --- a/udev/udev-node.c
> +++ b/udev/udev-node.c
> @@ -48,6 +48,7 @@ static int name_index(struct udev *udev, const char *devpath, >const char *name,
> dbg(udev, "creating index: '%s'\n", filename);
> util_create_path(udev, filename);
> fd = open(filename, O_WRONLY|O_TRUNC|O_CREAT, 0644);
> + util_create_path_done();
> if (fd > 0)
> close(fd);
> } else {
> @@ -356,6 +357,7 @@ static int update_link(struct udev_device *dev, const char >*slink)
> info(udev, "'%s' with target '%s' has the highest priority %i, create it\n", slink, >target, priority);
> util_create_path(udev, slink);
> node_symlink(udev, target, slink);
> + util_create_path_done();
> out:
> return rc;
> }
> @@ -456,6 +458,7 @@ int udev_node_add(struct udev_device *dev, mode_t mode, >uid_t uid, gid_t gid)
> update_link(dev, udev_list_entry_get_name(list_entry));
> }
> exit:
> + util_create_path_done();
> return err;
> }
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.
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
