| Message ID | 56409A52.2000708@commerceguys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 > >
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 > >
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
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;
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;