Karel Zak <[EMAIL PROTECTED]> writes:
> On Tue, Jun 19, 2007 at 12:58:06PM +0200, Matthias Koenig wrote:
>> Hi,
>>
>> in addition to the race described recently
>> (http://article.gmane.org/gmane.linux.utilities.util-linux-ng/241),
>> there is also a race if you have concurring processes running
>> mount -o loop. Patch below.
>
> Good idea! We can also add the same logic to the losetup.c:main().
Ok, I will send a separate patch for this.
>
> A few suggestions:
>
>> --- util-linux-devel.orig/mount/lomount.c
>> +++ util-linux-devel/mount/lomount.c
>> @@ -341,8 +341,15 @@ set_loop(const char *device, const char
>> }
>>
>> if (ioctl(fd, LOOP_SET_FD, ffd) < 0) {
>> - perror("ioctl: LOOP_SET_FD");
>> - return 1;
>> + switch (errno) {
>> + case EBUSY:
>> + if (verbose)
>> + perror("ioctl: LOOP_SET_FD");
> close (ffd);
Ah yes, and close(fd) is missing too.
>
>> + return 2;
>> + default:
>> + perror("ioctl: LOOP_SET_FD");
> close (ffd);
>
>> + return 1;
>> + }
>> }
>> close (ffd);
>
>
>> --- util-linux-devel.orig/mount/mount.c
>> +++ util-linux-devel/mount/mount.c
>> @@ -846,20 +846,34 @@ loop_check(const char **spec, const char
>
>
>> + if (!*loopdev || !**loopdev)
>> + *loopdev = find_unused_loop_device();
>> + if (!*loopdev)
>> + return EX_SYSERR; /* no more loop devices */
>> if (verbose)
>> - printf(_("mount: failed setting up loop device\n"));
>> - return EX_FAIL;
>> - }
>> + printf(_("mount: going to use the loop device %s\n"), *loopdev);
>> + offset = opt_offset ? strtoull(opt_offset, NULL, 0) : 0;
>> + if (res = set_loop (*loopdev, *loopfile, offset,
>> + opt_encryption, pfd, &loopro)) {
>> + switch(res) {
>> + case 2:
>> + /* loop dev has been grabbed by some other process,
>> + try again */
>> + if (verbose)
>> + printf("mount: stolen loop=%s ...trying again\n", *loopdev);
>> + *loopdev = NULL;
> ^^^^^^^^^^^^^^
> memory leak?
>
> free(*loopdev);
> *loopdev = NULL;
Right, find_unused_loop_device is using xstrdup
>
>> + continue;
> ^^^^^^^^
>
> Please, don't use the find_unused_loop_device() if user explicitly
> define a device.
>
> if (!opt_loop) {
> if (verbose)
> printf("mount: stolen loop=%s ...trying again\n", *loopdev);
> continue;
> }
> return EX_FAIL;
>
> Because:
>
> mount foo.img /foo -o loop=/dev/loop0
Ok.
Thanks for review, I will send a reworked patch.
Matthias
-
To unsubscribe from this list: send the line "unsubscribe util-linux-ng" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
