From 339b5cbcea1d459d2a0fc4d289e17fc71622be23 Mon Sep 17 00:00:00 2001

From: Jeffrey Cody <jcody@redhat.com>

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 <jcody@redhat.com>

Message-id: <cd1c834d5d15020c9521e95aa24d7db675dc12e2.1393961758.git.jcody@redhat.com>

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 <eblake@redhat.com>

RH-Acked-by: Amos Kong <akong@redhat.com>

RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

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 <mrezanin@redhat.com>

---

 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