btrfs-progs: Fix partitioned loop devices resolve.
diff mbox

Message ID 56409A52.2000708@commerceguys.com
State New
Headers show

Commit Message

Florian Margaine Nov. 9, 2015, 1:06 p.m. UTC
When using partitions on a loop device, the device's name can be
e.g. /dev/loop0p1 or similar, and no relevant entry exists in the /sys
filesystem, so the current resolve_loop_device function fails.

Instead of using string functions to extract the device name and reading
this file, this patch uses the loop device API through ioctl to get the
correct backing file.

Signed-off-by: Florian Margaine <florian@platform.sh>

---
 utils.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

 	return 0;

Comments

Karel Zak Nov. 9, 2015, 2:12 p.m. UTC | #1
On Mon, Nov 09, 2015 at 02:06:26PM +0100, Florian Margaine wrote:
> Instead of using string functions to extract the device name and reading
> this file, this patch uses the loop device API through ioctl to get the
> correct backing file.

    #define LO_NAME_SIZE    64

    struct loop_info64 {
        ...
        uint8_t         lo_file_name[LO_NAME_SIZE];
    };


The loopdev is based on file descriptor, the lo_file_name[] is hint
only and it does not have to match with the real path and the most
important problem is that it uses 64-bytes buffer.

For losetup we use LOOP_GET_STATUS64 ioctl as fallback solution only.

    Karel
Florian Margaine Nov. 9, 2015, 2:14 p.m. UTC | #2
On 11/09/2015 03:12 PM, Karel Zak wrote:
> On Mon, Nov 09, 2015 at 02:06:26PM +0100, Florian Margaine wrote:
>> Instead of using string functions to extract the device name and reading
>> this file, this patch uses the loop device API through ioctl to get the
>> correct backing file.
> 
>     #define LO_NAME_SIZE    64
> 
>     struct loop_info64 {
>         ...
>         uint8_t         lo_file_name[LO_NAME_SIZE];
>     };
> 
> 
> The loopdev is based on file descriptor, the lo_file_name[] is hint
> only and it does not have to match with the real path and the most
> important problem is that it uses 64-bytes buffer.
> 
> For losetup we use LOOP_GET_STATUS64 ioctl as fallback solution only.

So btrfs-progs should do the same? Use the /sys filesystem by default,
and use ioctl if it doesn't find the file?

> 
>     Karel
> 
>
Florian Margaine Nov. 10, 2015, 8:35 a.m. UTC | #3
On 11/09/2015 03:12 PM, Karel Zak wrote:
> On Mon, Nov 09, 2015 at 02:06:26PM +0100, Florian Margaine wrote:
>> Instead of using string functions to extract the device name and reading
>> this file, this patch uses the loop device API through ioctl to get the
>> correct backing file.
> 
>     #define LO_NAME_SIZE    64
> 
>     struct loop_info64 {
>         ...
>         uint8_t         lo_file_name[LO_NAME_SIZE];
>     };
> 
> 
> The loopdev is based on file descriptor, the lo_file_name[] is hint
> only and it does not have to match with the real path and the most
> important problem is that it uses 64-bytes buffer.
> 
> For losetup we use LOOP_GET_STATUS64 ioctl as fallback solution only.

I was thinking that this kind of code could be used, can you confirm
that this would be fine? Untested code:

static int resolve_loop_device()
{
    int ret;
    ret = fopen('/sys/...', 'r');
    if (ret == NULL)
        if (errno == ENOENT)
            return resolve_loop_device_ioctl();
}

static int __attribute__((noinline)) resolve_loop_device_ioctl()
{
    /* use ioctl */
}

This would use the normal path most of the time, and use the fallback
only if necessary. The 64-bytes buffer issue would be mitigated.

> 
>     Karel
> 
>
Karel Zak Nov. 10, 2015, 11:50 a.m. UTC | #4
On Tue, Nov 10, 2015 at 09:35:22AM +0100, Florian Margaine wrote:
> 
> 
> On 11/09/2015 03:12 PM, Karel Zak wrote:
> > On Mon, Nov 09, 2015 at 02:06:26PM +0100, Florian Margaine wrote:
> >> Instead of using string functions to extract the device name and reading
> >> this file, this patch uses the loop device API through ioctl to get the
> >> correct backing file.
> > 
> >     #define LO_NAME_SIZE    64
> > 
> >     struct loop_info64 {
> >         ...
> >         uint8_t         lo_file_name[LO_NAME_SIZE];
> >     };
> > 
> > 
> > The loopdev is based on file descriptor, the lo_file_name[] is hint
> > only and it does not have to match with the real path and the most
> > important problem is that it uses 64-bytes buffer.
> > 
> > For losetup we use LOOP_GET_STATUS64 ioctl as fallback solution only.
> 
> I was thinking that this kind of code could be used, can you confirm
> that this would be fine? Untested code:
> 
> static int resolve_loop_device()
> {
>     int ret;
>     ret = fopen('/sys/...', 'r');
>     if (ret == NULL)
>         if (errno == ENOENT)
>             return resolve_loop_device_ioctl();
> }
> 
> static int __attribute__((noinline)) resolve_loop_device_ioctl()
> {
>     /* use ioctl */
> }
> 
> This would use the normal path most of the time, and use the fallback
> only if necessary. The 64-bytes buffer issue would be mitigated.

Yep, first try /sys/... and when unsuccessful then try ioctl.

losetup example:
https://github.com/karelzak/util-linux/blob/master/lib/loopdev.c#L686

(it's probably too complex, but the basic idea is obvious)

Maybe we need libloop.so to share all these things between various
project :-)

    Karel

Patch
diff mbox

diff --git a/utils.c b/utils.c
index d546bea..73a05e1 100644
--- a/utils.c
+++ b/utils.c
@@ -1176,22 +1176,16 @@  static int is_loop_device (const char* device) {
 static int resolve_loop_device(const char* loop_dev, char* loop_file,
 		int max_len)
 {
-	int ret;
-	FILE *f;
-	char fmt[20];
-	char p[PATH_MAX];
-	char real_loop_dev[PATH_MAX];
+	int fd;
+	struct loop_info64 lo64;

-	if (!realpath(loop_dev, real_loop_dev))
+	if (!(fd = open(loop_dev, O_RDONLY)))
 		return -errno;
-	snprintf(p, PATH_MAX, "/sys/block/%s/loop/backing_file",
strrchr(real_loop_dev, '/'));
-	if (!(f = fopen(p, "r")))
+	if (ioctl(fd, LOOP_GET_STATUS64, &lo64) != 0)
 		return -errno;

-	snprintf(fmt, 20, "%%%i[^\n]", max_len-1);
-	ret = fscanf(f, fmt, loop_file);
-	fclose(f);
-	if (ret == EOF)
+	memcpy(loop_file, lo64.lo_file_name, strlen(lo64.lo_file_name) + 1);
+	if (close(fd) != 0)
 		return -errno;