From 339b5cbcea1d459d2a0fc4d289e17fc71622be23 Mon Sep 17 00:00:00 2001 From: Jeffrey Cody Date: Tue, 4 Mar 2014 19:38:27 +0100 Subject: [PATCH 6/6] block: Set block filename sizes to PATH_MAX instead of 1024 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RH-Author: Jeffrey Cody Message-id: Patchwork-id: 58013 O-Subject: [RHEL7 qemu-kvm PATCH] block: Set block filename sizes to PATH_MAX instead of 1024 Bugzilla: 1072339 RH-Acked-by: Eric Blake RH-Acked-by: Amos Kong RH-Acked-by: Stefan Hajnoczi This is an interim fix for a regression introduced by: commit dc5a137125d6ac641c566f10e68bf6e1fe31bcb5 qemu-img: make "info" backing file output correct and easier to use In that commit, bdrv_get_full_backing_filename() was introduced, which replaced calling path_combine() manually. The difference is that rather than using the filename string as passed to bdrv_open(), it uses the filename string attached to the BDS. Both the backing_file and filename strings in the BDS are limited to 1024 characters. The backing_file string built up in bdrv_open(), however, has a limit of PATH_MAX, which is 4096 under Linux. This difference comes into play when using an image chain that has a medium-to-large number of images, all of which have relative-pathed backing file references to the parent directory. For instance, consider the following backing chain: tstA ├── base.qcow2 ├── sn1.qcow2 (backing file ../tstA/base.qcow2) ├── sn2.qcow2 (backing file ../tstA/sn1.qcow2) └── sn3.qcow2 (backing file ../tstA/sn2.qcow2) The backing filename string is built up with the relative paths intact, like so: base.qcow2: ../tstA../tstA../tstA/base.qcow2 The backing filename is then passed into the bdrv_open() call to open the backing file. When using lv volume names, the snapshot and pathname ends up longer, and after ~23 snapshots we have hit or exceeded the 1024 byte limit for the BDS filename. This fix is different then the approach for RHEL6.6/6.5.z, because in those it was trivial to modify bdrv_get_full_backing_filename(). In RHEL7, there are places that use bdrv_get_backing_filename(), which call bdrv_get_full_backing_filename(), yet do not have access to a filename string that does not originate from the BDS. The simplest approach, that should yield identical results, is to set all of the filename and backing_file string sizes to PATH_MAX instead of 1024. This is not a long-term solution, because a character limit of 4096 bytes will be hit with additional images. The proper long-term solution should happen upstream first, and consist of: 1) dynamically allocated filename strings in the BDS 2) flattening redundant relative pathname strings This is a bug that was reported in RHEL6, that also occurs in RHEL7. To prevent a regression in RHEL7.0, this temporary solution will prevent the regression, while not eliminating the ultimate problem. Since this is not the final solution, and the fix really is relevant just to undo a regression, the fix is downstream only. It will be replaced by the final upstream fix, once complete. BZ 1072339 RHEL Brew: http://brewweb.devel.redhat.com/brew/taskinfo?taskID=7140291 RHEV Brew: http://brewweb.devel.redhat.com/brew/taskinfo?taskID=7140320 --- block/mirror.c | 2 +- block/qapi.c | 2 +- block/stream.c | 2 +- include/block/block_int.h | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) Signed-off-by: Miroslav Rezanina --- block/mirror.c | 2 +- block/qapi.c | 2 +- block/stream.c | 2 +- include/block/block_int.h | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index ba1428b..47e14cd 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -297,7 +297,7 @@ static void coroutine_fn mirror_run(void *opaque) int64_t sector_num, end, sectors_per_chunk, length; uint64_t last_pause_ns; BlockDriverInfo bdi; - char backing_filename[1024]; + char backing_filename[PATH_MAX]; int ret = 0; int n; diff --git a/block/qapi.c b/block/qapi.c index 77e1719..2d4bdcd 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -112,7 +112,7 @@ void bdrv_query_image_info(BlockDriverState *bs, { uint64_t total_sectors; const char *backing_filename; - char backing_filename2[1024]; + char backing_filename2[PATH_MAX]; BlockDriverInfo bdi; int ret; Error *err = NULL; diff --git a/block/stream.c b/block/stream.c index 3a7d8f3..2a6f533 100644 --- a/block/stream.c +++ b/block/stream.c @@ -32,7 +32,7 @@ typedef struct StreamBlockJob { RateLimit limit; BlockDriverState *base; BlockdevOnError on_error; - char backing_file_id[1024]; + char backing_file_id[PATH_MAX]; } StreamBlockJob; static int coroutine_fn stream_populate(BlockDriverState *bs, diff --git a/include/block/block_int.h b/include/block/block_int.h index 2ec4bb2..53fc98c 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -269,9 +269,9 @@ struct BlockDriverState { const BlockDevOps *dev_ops; void *dev_opaque; - char filename[1024]; - char backing_file[1024]; /* if non zero, the image is a diff of - this file image */ + char filename[PATH_MAX]; + char backing_file[PATH_MAX]; /* if non zero, the image is a diff of + this file image */ char backing_format[16]; /* if non-zero and backing_file exists */ int is_temporary; -- 1.7.1