btrfs-progs: Fix partitioned loop devices resolve.
PATCH 1/1 btrfs-progs: Fix partitioned loop devices resolve.
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.
--- utils.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
utils.c +3 -7
@@ -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;
Signature · 1 line
2.6.1
Quoted 3 lines · Florian Margaine
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
Signature · 2 lines
Karel Zak <[email protected]> http://karelzak.blogspot.com
Quoted 13 lines · Karel Zak
Quoted 3 lines · Florian Margaine
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]; };
Quoted 5 lines
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_STATUS64ioctl 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?
Quoted 4 lines
Karel
Signature · 6 lines
Florian Margaine Product Engineer @ Platform.sh https://platform.sh https://keybase.io/fmargaine
Quoted 13 lines · Karel Zak
Quoted 3 lines · Florian Margaine
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]; };
Quoted 5 lines
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_STATUS64ioctl 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.
Quoted 4 lines
Karel
Signature · 6 lines
Florian Margaine Product Engineer @ Platform.sh https://platform.sh https://keybase.io/fmargaine
Quoted 34 lines · Florian Margaine
Quoted 13 lines · Karel Zak
Quoted 3 lines · Florian Margaine
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]; };Quoted 5 lines
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_STATUS64ioctl 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(); }
Quoted 7 lines
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
Signature · 2 lines
Karel Zak <[email protected]> http://karelzak.blogspot.com
New patch is attached.
Quoted 10 lines · Karel Zak
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.
Quoted 3 lines
Karel
Regards,
Signature · 6 lines
Florian Margaine Product Engineer @ Platform.sh https://platform.sh https://keybase.io/fmargaine
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(-)
utils.c +0 -0
@@ -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
Quoted 1 line · Florian Margaine
New patch is attached.
Applied with some modifications, thanks.
Review
Quoted 11 lines
(quoted diff) +11
-0+/* + * 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
Review
Quoted 2 lines
(quoted diff) +2
-0+ return -errno; + if (ioctl(fd, LOOP_GET_STATUS64, &lo64) != 0)
open and ioctl return < 0 in case of error
Quoted 12 lines
(quoted diff) +8
-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) */
Review static int resolve_loop_device(const char* loop_dev, char* loop_file, · line 1212
Quoted 8 lines
(quoted diff) +4
-1@@ -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
Quoted 6 lines
(quoted diff) +2
-0+ return resolve_loop_device_with_loopdev(loop_dev, loop_file); return -errno; + }(quoted diff) +0
-0snprintf(fmt, 20, "%%%i[^\n]", max_len-1); ret = fscanf(f, fmt, loop_file);