[PATCH] btrfs-progs: Fix partitioned loop devices resolve.

[PATCH] btrfs-progs: Fix partitioned loop devices resolve.

[Florian Margaine]

[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

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(-)

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;

 	return 0;
-- 
2.6.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] btrfs-progs: Fix partitioned loop devices resolve.

[Karel Zak]

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


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] btrfs-progs: Fix partitioned loop devices resolve.

[Florian Margaine]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]



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

Product Engineer @ Platform.sh
https://platform.sh

https://keybase.io/fmargaine


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] btrfs-progs: Fix partitioned loop devices resolve.

[Florian Margaine]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]



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
> 
> 

-- 
Florian Margaine

Product Engineer @ Platform.sh
https://platform.sh

https://keybase.io/fmargaine


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] btrfs-progs: Fix partitioned loop devices resolve.

[Karel Zak]

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

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] btrfs-progs: Fix partitioned loop devices resolve.

[Florian Margaine]

[-- Attachment #1.1: Type: text/plain, Size: 568 bytes --]

New patch is attached.

On 11/10/2015 12:50 PM, Karel Zak wrote:
> 
> 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 :-)

That would be lovely indeed.

> 
>     Karel
> 

Regards,

-- 
Florian Margaine

Product Engineer @ Platform.sh
https://platform.sh

https://keybase.io/fmargaine

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: fix-partitioned-loop-devices-resolve.patch --]
[-- Type: text/x-patch; name="fix-partitioned-loop-devices-resolve.patch", Size: 2001 bytes --]

When using partitions on a loop device, the device's name can be
/dev/loop0p1 or similar, and no relevant entry exists in the /sys
filesystem, so the current resolve_loop_device function fails.

This patch adds a fallback to using loopdev API which will get the
correct backing file even for partitioned loop devices, at the expense
of using more memory, hence using it as a fallback only.
---
 utils.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/utils.c b/utils.c
index d546bea..5e4813d 100644
--- a/utils.c
+++ b/utils.c
@@ -1170,2 +1170,2 @@ static int is_loop_device (const char* device) {
 		MAJOR(statbuf.st_rdev) == LOOP_MAJOR);
 }

+/*
+ * Takes a loop device path (e.g. /dev/loop0) and returns
+ * the associated file (e.g. /images/my_btrfs.img) using
+ * loopdev API
+ */
+static int resolve_loop_device_with_loopdev(const char* loop_dev, char* loop_file)
+{
+	int fd;
+	struct loop_info64 lo64;
+
+	if (!(fd = open(loop_dev, O_RDONLY | O_NONBLOCK)))
+		return -errno;
+	if (ioctl(fd, LOOP_GET_STATUS64, &lo64) != 0)
+		return -errno;
+
+	memcpy(loop_file, lo64.lo_file_name, strlen(lo64.lo_file_name) + 1);
+	if (close(fd) != 0)
+		return -errno;
+
+	return 0;
+}

 /* Takes a loop device path (e.g. /dev/loop0) and returns
  * the associated file (e.g. /images/my_btrfs.img) */
@@ -1185,5 +1206,10 @@ static int resolve_loop_device(const char* loop_dev, char* loop_file,
 	if (!realpath(loop_dev, real_loop_dev))
 		return -errno;
 	snprintf(p, PATH_MAX, "/sys/block/%s/loop/backing_file", strrchr(real_loop_dev, '/'));
-	if (!(f = fopen(p, "r")))
+	if (!(f = fopen(p, "r"))) {
+		if (errno == ENOENT)
+			/* It's possibly a partitioned loop device, which
+			   is resolvable with loopdev API. */
+			return resolve_loop_device_with_loopdev(loop_dev, loop_file);
 		return -errno;
+	}

 	snprintf(fmt, 20, "%%%i[^\n]", max_len-1);
 	ret = fscanf(f, fmt, loop_file);
--
2.6.1

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] btrfs-progs: Fix partitioned loop devices resolve.

[David Sterba]

On Thu, Nov 12, 2015 at 10:10:51AM +0100, Florian Margaine wrote:
> New patch is attached.

Applied with some modifications, thanks.

> +/*
> + * Takes a loop device path (e.g. /dev/loop0) and returns
> + * the associated file (e.g. /images/my_btrfs.img) using
> + * loopdev API
> + */
> +static int resolve_loop_device_with_loopdev(const char* loop_dev, char* loop_file)
> +{
> +	int fd;
> +	struct loop_info64 lo64;
> +
> +	if (!(fd = open(loop_dev, O_RDONLY | O_NONBLOCK)))

fd = ... moved before if

> +		return -errno;
> +	if (ioctl(fd, LOOP_GET_STATUS64, &lo64) != 0)

open and ioctl return < 0 in case of error

> +		return -errno;
> +
> +	memcpy(loop_file, lo64.lo_file_name, strlen(lo64.lo_file_name) + 1);
> +	if (close(fd) != 0)
> +		return -errno;
> +
> +	return 0;
> +}
> 
>  /* Takes a loop device path (e.g. /dev/loop0) and returns
>   * the associated file (e.g. /images/my_btrfs.img) */
> @@ -1185,5 +1206,10 @@ static int resolve_loop_device(const char* loop_dev, char* loop_file,
>  	if (!realpath(loop_dev, real_loop_dev))
>  		return -errno;
>  	snprintf(p, PATH_MAX, "/sys/block/%s/loop/backing_file", strrchr(real_loop_dev, '/'));
> -	if (!(f = fopen(p, "r")))
> +	if (!(f = fopen(p, "r"))) {
> +		if (errno == ENOENT)
> +			/* It's possibly a partitioned loop device, which
> +			   is resolvable with loopdev API. */

comment style fixed

> +			return resolve_loop_device_with_loopdev(loop_dev, loop_file);
>  		return -errno;
> +	}
> 
>  	snprintf(fmt, 20, "%%%i[^\n]", max_len-1);
>  	ret = fscanf(f, fmt, loop_file);