On 10/7/19 11:49 PM, Cole Robinson wrote:

Check explicitly for BACKING_STORE_OK and not its 0 value

Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/virstoragefile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 51726006e7..1549067c48 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -578,7 +578,7 @@ qcow1GetBackingStore(char **res, * used to store backing format */ *format = VIR_STORAGE_FILE_AUTO; ret = qcowXGetBackingStore(res, NULL, buf, buf_size, false); - if (ret == 0 && *buf == '\0') + if (ret == BACKING_STORE_OK && *buf == '\0') *format = VIR_STORAGE_FILE_NONE; return ret; }

We can make qcowXGetBackingStore() return the enum type instead of plain int. But that can be done in a follow up (trivial) patch. When doing that, both qcow1GetBackingStore() and qcow2GetBackingStore() and also getBackingStore() callback can use the same tretement then. // after seeing future patches Ah, you're removing some functions, but you get the idea. Michal

On 10/7/19 6:49 PM, Cole Robinson wrote:

Letting qcowXGetBackingStore fill in format gives the same behavior we were opencoding in qcow1GetBackingStore

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/util/virstoragefile.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 016c8f0799..905e70b1a9 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -494,8 +494,7 @@ qcowXGetBackingStore(char **res, int version;

*res = NULL; - if (format) - *format = VIR_STORAGE_FILE_AUTO; + *format = VIR_STORAGE_FILE_AUTO;

if (buf_size < QCOWX_HDR_BACKING_FILE_OFFSET+8+4) return BACKING_STORE_INVALID; @@ -504,15 +503,13 @@ qcowXGetBackingStore(char **res, return BACKING_STORE_INVALID;

if (offset == 0) { - if (format) - *format = VIR_STORAGE_FILE_NONE; + *format = VIR_STORAGE_FILE_NONE; return BACKING_STORE_OK; }

size = virReadBufInt32BE(buf + QCOWX_HDR_BACKING_FILE_SIZE); if (size == 0) { - if (format) - *format = VIR_STORAGE_FILE_NONE; + *format = VIR_STORAGE_FILE_NONE; return BACKING_STORE_OK; } if (size > 1023) @@ -551,7 +548,7 @@ qcowXGetBackingStore(char **res, * for qcow2 v3 images, the length of the header * is stored at QCOW2v3_HDR_SIZE */ - if (isQCow2 && format) { + if (isQCow2) { version = virReadBufInt32BE(buf + QCOWX_HDR_VERSION); if (version == 2) start = QCOW2_HDR_TOTAL_SIZE; @@ -572,15 +569,9 @@ qcow1GetBackingStore(char **res, const char *buf, size_t buf_size) { - int ret; - /* QCow1 doesn't have the extensions capability * used to store backing format */ - *format = VIR_STORAGE_FILE_AUTO; - ret = qcowXGetBackingStore(res, NULL, buf, buf_size, false); - if (ret == BACKING_STORE_OK && !*res) - *format = VIR_STORAGE_FILE_NONE; - return ret; + return qcowXGetBackingStore(res, format, buf, buf_size, false); }

static int

On 10/7/19 6:49 PM, Cole Robinson wrote:

Rather than require a boolean to be passed in

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

I was about to say 'why didn't you remove the isQCow2 boolean and change the 2 callers of the function ' but then I saw that the next 2 patches does further code simplification here. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/util/virstoragefile.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 905e70b1a9..9bf4c1178b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -486,7 +486,7 @@ qcowXGetBackingStore(char **res, int *format, const char *buf, size_t buf_size, - bool isQCow2) + bool isQCow2 ATTRIBUTE_UNUSED) { unsigned long long offset; unsigned int size; @@ -548,8 +548,11 @@ qcowXGetBackingStore(char **res, * for qcow2 v3 images, the length of the header * is stored at QCOW2v3_HDR_SIZE */ - if (isQCow2) { - version = virReadBufInt32BE(buf + QCOWX_HDR_VERSION); + + version = virReadBufInt32BE(buf + QCOWX_HDR_VERSION); + if (version >= 2) { + /* QCow1 doesn't have the extensions capability + * used to store backing format */ if (version == 2) start = QCOW2_HDR_TOTAL_SIZE; else @@ -569,8 +572,6 @@ qcow1GetBackingStore(char **res, const char *buf, size_t buf_size) { - /* QCow1 doesn't have the extensions capability - * used to store backing format */ return qcowXGetBackingStore(res, format, buf, buf_size, false); }

On 10/7/19 6:49 PM, Cole Robinson wrote:

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/util/virstoragefile.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 9bf4c1178b..14551af4d2 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -485,8 +485,7 @@ static int qcowXGetBackingStore(char **res, int *format, const char *buf, - size_t buf_size, - bool isQCow2 ATTRIBUTE_UNUSED) + size_t buf_size) { unsigned long long offset; unsigned int size; @@ -572,7 +571,7 @@ qcow1GetBackingStore(char **res, const char *buf, size_t buf_size) { - return qcowXGetBackingStore(res, format, buf, buf_size, false); + return qcowXGetBackingStore(res, format, buf, buf_size); }

static int @@ -581,7 +580,7 @@ qcow2GetBackingStore(char **res, const char *buf, size_t buf_size) { - return qcowXGetBackingStore(res, format, buf, buf_size, true); + return qcowXGetBackingStore(res, format, buf, buf_size); }

On 10/7/19 6:49 PM, Cole Robinson wrote:

The qcow1 and qcow2 variants are identical, so remove the wrappers

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/util/virstoragefile.c | 27 +++------------------------ 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 14551af4d2..a9a6c3e132 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -180,9 +180,7 @@ struct FileTypeInfo {

static int cowGetBackingStore(char **, int *, const char *, size_t); -static int qcow1GetBackingStore(char **, int *, - const char *, size_t); -static int qcow2GetBackingStore(char **, int *, +static int qcowXGetBackingStore(char **, int *, const char *, size_t); static int qcow2GetFeatures(virBitmapPtr *features, int format, char *buf, ssize_t len); @@ -366,14 +364,14 @@ static struct FileTypeInfo const fileTypeInfo[] = { LV_BIG_ENDIAN, 4, 4, {1}, QCOWX_HDR_IMAGE_SIZE, 8, 1, qcow1EncryptionInfo, - qcow1GetBackingStore, NULL + qcowXGetBackingStore, NULL }, [VIR_STORAGE_FILE_QCOW2] = { 0, "QFI", NULL, LV_BIG_ENDIAN, 4, 4, {2, 3}, QCOWX_HDR_IMAGE_SIZE, 8, 1, qcow2EncryptionInfo, - qcow2GetBackingStore, + qcowXGetBackingStore, qcow2GetFeatures }, [VIR_STORAGE_FILE_QED] = { @@ -565,25 +563,6 @@ qcowXGetBackingStore(char **res, }

-static int -qcow1GetBackingStore(char **res, - int *format, - const char *buf, - size_t buf_size) -{ - return qcowXGetBackingStore(res, format, buf, buf_size); -} - -static int -qcow2GetBackingStore(char **res, - int *format, - const char *buf, - size_t buf_size) -{ - return qcowXGetBackingStore(res, format, buf, buf_size); -} - - static int vmdk4GetBackingStore(char **res, int *format,

On 10/7/19 6:49 PM, Cole Robinson wrote:

This is a step towards making this qcow2GetBackingStoreFormat into a generic qcow2 extensions parser

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/util/virstoragefile.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a9a6c3e132..4a3c9df7a2 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -430,10 +430,22 @@ static int qcow2GetBackingStoreFormat(int *format, const char *buf, size_t buf_size, - size_t extension_start, size_t extension_end) { - size_t offset = extension_start; + size_t offset; + size_t extension_start; + int version = virReadBufInt32BE(buf + QCOWX_HDR_VERSION); + + if (version < 2) { + /* QCow1 doesn't have the extensions capability + * used to store backing format */ + return 0; + } + + if (version == 2) + extension_start = QCOW2_HDR_TOTAL_SIZE; + else + extension_start = virReadBufInt32BE(buf + QCOW2v3_HDR_SIZE);

/* * The extensions take format of @@ -445,6 +457,7 @@ qcow2GetBackingStoreFormat(int *format, * Unknown extensions can be ignored by skipping * over "length" bytes in the data stream. */ + offset = extension_start; while (offset < (buf_size-8) && offset < (extension_end-8)) { unsigned int magic = virReadBufInt32BE(buf + offset); @@ -487,8 +500,6 @@ qcowXGetBackingStore(char **res, { unsigned long long offset; unsigned int size; - unsigned long long start; - int version;

*res = NULL; *format = VIR_STORAGE_FILE_AUTO; @@ -546,18 +557,8 @@ qcowXGetBackingStore(char **res, * is stored at QCOW2v3_HDR_SIZE */

- version = virReadBufInt32BE(buf + QCOWX_HDR_VERSION); - if (version >= 2) { - /* QCow1 doesn't have the extensions capability - * used to store backing format */ - if (version == 2) - start = QCOW2_HDR_TOTAL_SIZE; - else - start = virReadBufInt32BE(buf + QCOW2v3_HDR_SIZE); - if (qcow2GetBackingStoreFormat(format, buf, buf_size, - start, offset) < 0) - return BACKING_STORE_INVALID; - } + if (qcow2GetBackingStoreFormat(format, buf, buf_size, offset) < 0) + return BACKING_STORE_INVALID;

return BACKING_STORE_OK; }

On 10/7/19 6:49 PM, Cole Robinson wrote:

This is a step towards making this qcow2GetBackingStoreFormat into a generic qcow2 extensions parser

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/util/virstoragefile.c | 66 +++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 4a3c9df7a2..b8f7faf580 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -429,11 +429,11 @@ cowGetBackingStore(char **res, static int qcow2GetBackingStoreFormat(int *format, const char *buf, - size_t buf_size, - size_t extension_end) + size_t buf_size) { size_t offset; size_t extension_start; + size_t extension_end; int version = virReadBufInt32BE(buf + QCOWX_HDR_VERSION);

if (version < 2) { @@ -447,6 +447,37 @@ qcow2GetBackingStoreFormat(int *format, else extension_start = virReadBufInt32BE(buf + QCOW2v3_HDR_SIZE);

+ /* + * Traditionally QCow2 files had a layout of + * + * [header] + * [backingStoreName] + * + * Although the backingStoreName typically followed + * the header immediately, this was not required by + * the format. By specifying a higher byte offset for + * the backing file offset in the header, it was + * possible to leave space between the header and + * start of backingStore. + * + * This hack is now used to store extensions to the + * qcow2 format: + * + * [header] + * [extensions] + * [backingStoreName] + * + * Thus the file region to search for extensions is + * between the end of the header (QCOW2_HDR_TOTAL_SIZE) + * and the start of the backingStoreName (offset) + * + * for qcow2 v3 images, the length of the header + * is stored at QCOW2v3_HDR_SIZE + */ + extension_end = virReadBufInt64BE(buf + QCOWX_HDR_BACKING_FILE_OFFSET); + if (extension_end > buf_size) + return -1; + /* * The extensions take format of * @@ -506,6 +537,7 @@ qcowXGetBackingStore(char **res,

if (buf_size < QCOWX_HDR_BACKING_FILE_OFFSET+8+4) return BACKING_STORE_INVALID; + offset = virReadBufInt64BE(buf + QCOWX_HDR_BACKING_FILE_OFFSET); if (offset > buf_size) return BACKING_STORE_INVALID; @@ -529,35 +561,7 @@ qcowXGetBackingStore(char **res, memcpy(*res, buf + offset, size); (*res)[size] = '\0';

- /* - * Traditionally QCow2 files had a layout of - * - * [header] - * [backingStoreName] - * - * Although the backingStoreName typically followed - * the header immediately, this was not required by - * the format. By specifying a higher byte offset for - * the backing file offset in the header, it was - * possible to leave space between the header and - * start of backingStore. - * - * This hack is now used to store extensions to the - * qcow2 format: - * - * [header] - * [extensions] - * [backingStoreName] - * - * Thus the file region to search for extensions is - * between the end of the header (QCOW2_HDR_TOTAL_SIZE) - * and the start of the backingStoreName (offset) - * - * for qcow2 v3 images, the length of the header - * is stored at QCOW2v3_HDR_SIZE - */ - - if (qcow2GetBackingStoreFormat(format, buf, buf_size, offset) < 0) + if (qcow2GetBackingStoreFormat(format, buf, buf_size) < 0) return BACKING_STORE_INVALID;

return BACKING_STORE_OK;

On 10/7/19 6:49 PM, Cole Robinson wrote:

...to qcow2GetExtensions. We will extend it for more extension parsing in future patches

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/util/virstoragefile.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b8f7faf580..d6bd38777b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -427,9 +427,9 @@ cowGetBackingStore(char **res,

static int -qcow2GetBackingStoreFormat(int *format, - const char *buf, - size_t buf_size) +qcow2GetExtensions(int *format, + const char *buf, + size_t buf_size) { size_t offset; size_t extension_start; @@ -561,7 +561,7 @@ qcowXGetBackingStore(char **res, memcpy(*res, buf + offset, size); (*res)[size] = '\0';

- if (qcow2GetBackingStoreFormat(format, buf, buf_size) < 0) + if (qcow2GetExtensions(format, buf, buf_size) < 0) return BACKING_STORE_INVALID;

return BACKING_STORE_OK;

On 10/7/19 6:49 PM, Cole Robinson wrote:

To backingFormat, which makes it more clear. Move it to the end of the argument list which will scale nicer with future patches

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

This really makes it clearer. I was getting confused about whether 'format' was referring to the image or its backing file in these patches.

src/util/virstoragefile.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d6bd38777b..8cd576a463 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -427,9 +427,9 @@ cowGetBackingStore(char **res,

static int -qcow2GetExtensions(int *format, - const char *buf, - size_t buf_size) +qcow2GetExtensions(const char *buf, + size_t buf_size, + int *backingFormat) { size_t offset; size_t extension_start; @@ -509,8 +509,8 @@ qcow2GetExtensions(int *format, case QCOW2_HDR_EXTENSION_BACKING_FORMAT: if (buf[offset+len] != '\0') break; - *format = virStorageFileFormatTypeFromString(buf+offset); - if (*format <= VIR_STORAGE_FILE_NONE) + *backingFormat = virStorageFileFormatTypeFromString(buf+offset); + if (*backingFormat <= VIR_STORAGE_FILE_NONE) return -1; }

@@ -561,7 +561,7 @@ qcowXGetBackingStore(char **res, memcpy(*res, buf + offset, size); (*res)[size] = '\0';

- if (qcow2GetExtensions(format, buf, buf_size) < 0) + if (qcow2GetExtensions(buf, buf_size, format) < 0) return BACKING_STORE_INVALID;

Not sure if the next patches are changing it or making it obsolete, but you can change 'format' to 'backingFormat' inside qcowXGetBackingStore too. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

return BACKING_STORE_OK;

On 10/9/19 5:34 PM, Daniel Henrique Barboza wrote:

On 10/7/19 6:49 PM, Cole Robinson wrote:

...

...

From qemu.git docs/interop/qcow2.txt

Here's the '>' again. I think this is something you're using to cite an external source in the commit message. Is that it?

...

   == String header extensions ==

   Some header extensions (such as the backing file format name and    the external data file name) are just a single string. In this case,    the header extension length is the string length and the string is    not '\0' terminated. (The header extension padding can make it look    like a string is '\0' terminated, but neither is padding always    necessary nor is there a guarantee that zero bytes are used    for padding.)

So we shouldn't be checking for a \0 byte at the end of the backing format section. I think in practice there always is a \0 but we shouldn't depend on that.

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---   src/util/virstoragefile.c | 13 +++++++++----   1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8cd576a463..5a4e4b24ae 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -506,13 +506,18 @@ qcow2GetExtensions(const char *buf,           case QCOW2_HDR_EXTENSION_END:               goto done; -        case QCOW2_HDR_EXTENSION_BACKING_FORMAT: -            if (buf[offset+len] != '\0') -                break; -            *backingFormat = virStorageFileFormatTypeFromString(buf+offset); +        case QCOW2_HDR_EXTENSION_BACKING_FORMAT: { +            VIR_AUTOFREE(char *) tmp = NULL; +            if (VIR_ALLOC_N(tmp, len + 1) < 0) +                return -1; +            memcpy(tmp, buf + offset, len); +            tmp[len] = '\0'; + +            *backingFormat = virStorageFileFormatTypeFromString(tmp);               if (*backingFormat <= VIR_STORAGE_FILE_NONE)                   return -1;           } +        }

I suppose this extra brace is due to the new brace right after label that you added up there. I'm probably being a purist here, but I'd rather make an extra indent level in each 'case' statement of this switch just to avoid this  braces right below each other. We have this style of switch statement in the code, so I think it would be ok.

That's my personal inclination as well, but a 'git grep -A1 "switch ("' shows that the vast majority of switch statements don't indent the case. The weirdness with the bracket here is just a side effect of that. I swapped the case blocks around so at least the brackets are on adjacent lines :) Thanks, Cole

On 10/7/19 11:49 PM, Cole Robinson wrote:

...

From qemu.git docs/interop/qcow2.txt

== String header extensions ==

Some header extensions (such as the backing file format name and the external data file name) are just a single string. In this case, the header extension length is the string length and the string is not '\0' terminated. (The header extension padding can make it look like a string is '\0' terminated, but neither is padding always necessary nor is there a guarantee that zero bytes are used for padding.)

So we shouldn't be checking for a \0 byte at the end of the backing format section. I think in practice there always is a \0 but we shouldn't depend on that.

Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/virstoragefile.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 8cd576a463..5a4e4b24ae 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -506,13 +506,18 @@ qcow2GetExtensions(const char *buf, case QCOW2_HDR_EXTENSION_END: goto done;

- case QCOW2_HDR_EXTENSION_BACKING_FORMAT: - if (buf[offset+len] != '\0') - break; - *backingFormat = virStorageFileFormatTypeFromString(buf+offset); + case QCOW2_HDR_EXTENSION_BACKING_FORMAT: { + VIR_AUTOFREE(char *) tmp = NULL; + if (VIR_ALLOC_N(tmp, len + 1) < 0) + return -1; + memcpy(tmp, buf + offset, len); + tmp[len] = '\0'; + + *backingFormat = virStorageFileFormatTypeFromString(tmp); if (*backingFormat <= VIR_STORAGE_FILE_NONE) return -1; } + }

Pre-existing, but there's missing 'break;' here. Since you're touching these lines, might be worth putting it here.

offset += len; }

Michal

On 10/10/19 5:09 PM, Daniel Henrique Barboza wrote:

On 10/7/19 6:49 PM, Cole Robinson wrote:

...

Add the plumbing to track a externalDataStoreRaw as a virStorageSource

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---   src/util/virstoragefile.c | 9 +++++++++   src/util/virstoragefile.h | 3 +++   2 files changed, 12 insertions(+)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 7ae6719dd6..ce669b6e0b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2339,6 +2339,12 @@ virStorageSourceCopy(const virStorageSource *src,               return NULL;       } +    if (src->externalDataStore) { +        if (!(def->externalDataStore = virStorageSourceCopy(src->externalDataStore, +                                                            true))) +            return NULL; +    } +       VIR_STEAL_PTR(ret, def);       return ret;   } @@ -2560,6 +2566,9 @@ virStorageSourceClear(virStorageSourcePtr def)       VIR_FREE(def->timestamps);       VIR_FREE(def->externalDataStoreRaw); +    virObjectUnref(def->externalDataStore); +    def->externalDataStore = NULL;

Is this assign to NULL necessary? virObjectUnref will call VIR_FREE() in def->externalDataStore if there is no more references to it (which will assign it to NULL in the end).

It is. Point of virXXXClear() functions is to clear passed struct. That is, in theory (I'm not saying it's the case of virStorageSourceDef and also I'm not saying it isn't), it should be possible to re-use a structure after it's been cleared. But for that to happen, all poitners must be reset to NULL regardless of refcounters and stuff. However, there's a memset() called at the end of this function which sets all members of this struct to 0, so in the end I agree that the line is redundant, but for a different reason :-D Michal

On 10/7/19 6:49 PM, Cole Robinson wrote:

The only caller always passes in a non-null parent

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

A replay from patch 20. I wonder how much common code there are between security_dac.c and security_selinux.c. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/security/security_selinux.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 9d28bc5773..e384542c49 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1849,9 +1849,8 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,

disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); - if (parent) - parent_seclabel = virStorageSourceGetSecurityLabelDef(parent, - SECURITY_SELINUX_NAME); + parent_seclabel = virStorageSourceGetSecurityLabelDef(parent, + SECURITY_SELINUX_NAME);

if (disk_seclabel && (!disk_seclabel->relabel || disk_seclabel->label)) { if (!disk_seclabel->relabel) @@ -1863,7 +1862,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, return 0;

use_label = parent_seclabel->label; - } else if (!parent || parent == src) { + } else if (parent == src) { if (src->shared) { use_label = data->file_context; } else if (src->readonly) {

On 10/7/19 6:49 PM, Cole Robinson wrote:

This will simplify future patches and make the logic easier to follow

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/security/security_selinux.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e384542c49..fd7dd080c1 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1824,6 +1824,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, virSecurityDeviceLabelDefPtr parent_seclabel = NULL; char *use_label = NULL; bool remember; + bool is_toplevel = parent == src; int ret;

if (!src->path || !virStorageSourceIsLocalStorage(src)) @@ -1845,7 +1846,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, * but the top layer, or read only image, or disk explicitly * marked as shared. */ - remember = src == parent && !src->readonly && !src->shared; + remember = is_toplevel && !src->readonly && !src->shared;

disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); @@ -1862,7 +1863,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, return 0;

use_label = parent_seclabel->label; - } else if (parent == src) { + } else if (is_toplevel) { if (src->shared) { use_label = data->file_context; } else if (src->readonly) {

On 10/7/19 6:49 PM, Cole Robinson wrote:

Rename the existing virSecuritySELinuxRestoreImageLabelInt to virSecuritySELinuxRestoreImageLabelSingle, and extend the new ImageLabelInt handle externalDataStore

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/security/security_selinux.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index fd7dd080c1..c0bfb581e3 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1747,10 +1747,10 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManagerPtr mgr,

static int -virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virStorageSourcePtr src, - bool migrated) +virSecuritySELinuxRestoreImageLabelSingle(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src, + bool migrated) { virSecurityLabelDefPtr seclabel; virSecurityDeviceLabelDefPtr disk_seclabel; @@ -1802,6 +1802,26 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, }

+static int +virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src, + bool migrated) +{ + if (virSecuritySELinuxRestoreImageLabelSingle(mgr, def, src, migrated) < 0) + return -1; + + if (src->externalDataStore && + virSecuritySELinuxRestoreImageLabelSingle(mgr, + def, + src->externalDataStore, + migrated) < 0) + return -1; + + return 0; +} + + static int virSecuritySELinuxRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def,

On 10/7/19 6:49 PM, Cole Robinson wrote:

This will be used for recursing into externalDataStore

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/security/security_selinux.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c0bfb581e3..feb703d325 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1919,15 +1919,16 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,

static int -virSecuritySELinuxSetImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virStorageSourcePtr src, - virSecurityDomainImageLabelFlags flags) +virSecuritySELinuxSetImageLabelRelative(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src, + virStorageSourcePtr parent, + virSecurityDomainImageLabelFlags flags) { virStorageSourcePtr n;

for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) { - if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, src) < 0) + if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent) < 0) return -1;

if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN)) @@ -1938,6 +1939,15 @@ virSecuritySELinuxSetImageLabel(virSecurityManagerPtr mgr, }

+static int +virSecuritySELinuxSetImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src, + virSecurityDomainImageLabelFlags flags) +{ + return virSecuritySELinuxSetImageLabelRelative(mgr, def, src, src, flags); +} + struct virSecuritySELinuxMoveImageMetadataData { virSecurityManagerPtr mgr; const char *src;

On 10/7/19 6:49 PM, Cole Robinson wrote:

We mirror the labeling strategy that was used for its top image

Signed-off-by: Cole Robinson <crobinso@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

src/security/security_selinux.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index feb703d325..2a3b7fc10d 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1844,7 +1844,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr, virSecurityDeviceLabelDefPtr parent_seclabel = NULL; char *use_label = NULL; bool remember; - bool is_toplevel = parent == src; + bool is_toplevel = parent == src || parent->externalDataStore == src; int ret;

if (!src->path || !virStorageSourceIsLocalStorage(src)) @@ -1931,6 +1931,14 @@ virSecuritySELinuxSetImageLabelRelative(virSecurityManagerPtr mgr, if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent) < 0) return -1;

+ if (n->externalDataStore && + virSecuritySELinuxSetImageLabelRelative(mgr, + def, + n->externalDataStore, + parent, + flags) < 0) + return -1; + if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN)) break; }

On 10/8/19 1:52 AM, Christian Ehrhardt wrote:

On Mon, Oct 7, 2019 at 11:49 PM Cole Robinson <crobinso@redhat.com> wrote:

...

This series is the first steps to teaching libvirt about qcow2 data_file support, aka external data files or qcow2 external metadata.

A bit about the feature: it was added in qemu 4.0. It essentially creates a two part image file: a qcow2 layer that just tracks the image metadata, and a separate data file which is stores the VM disk contents. AFAICT the driving use case is to keep a fully coherent raw disk image on disk, and only use qcow2 as an intermediate metadata layer when necessary, for things like incremental backup support.

The original qemu patch posting is here: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07496.html

For testing, you can create a new qcow2+raw data_file image from an existing image, like:

qemu-img convert -O qcow2 \ -o data_file=NEW.raw,data_file_raw=yes EXISTING.raw NEW.qcow2

The goal of this series is to teach libvirt enough about this case so that we can correctly relabel the data_file on VM startup/shutdown. The main functional changes are

* Teach storagefile how to parse out data_file from the qcow2 header * Store the raw string as virStorageSource->externalDataStoreRaw * Track that as its out virStorageSource in externalDataStore * dac/selinux relabel externalDataStore as needed

...

From libvirt's perspective, externalDataStore is conceptually pretty close to a backingStore, but the main difference is its read/write permissions should match its parent image, rather than being readonly like backingStore.

This series has only been tested on top of the -blockdev enablement series, but I don't think it actually interacts with that work at the moment.

Future work: * Exposing this in the runtime XML. We need to figure out an XML schema. It will reuse virStorageSource obviously, but the main thing to figure out is probably 1) what the top element name should be ('dataFile' maybe?), 2) where it sits in the XML hierarchy (under <disk> or under <source> I guess)

* Exposing this on the qemu -blockdev command line. Similar to how in the blockdev world we are explicitly putting the disk backing chain on the command line, we can do that for data_file too. Then like persistent <backingStore> XML the user will have the power to overwrite the data_file location for an individual VM run.

* Figure out how we expect ovirt/rhev to be using this at runtime. Possibly taking a running VM using a raw image, doing blockdev-* magic to pivot it to qcow2+raw data_file, so it can initiate incremental backup on top of a previously raw only VM?

Known issues: * In the qemu driver, the qcow2 image metadata is only parsed in -blockdev world if no <backingStore> is specified in the persistent XML. So basically if there's a <backingStore> listed, we never parse the qcow2 header and detect the presence of data_file. Fixable I'm sure but I didn't look into it much yet.

Most of this is cleanups and refactorings to simplify the actual functional changes.

Cole Robinson (30): storagefile: Make GetMetadataInternal static storagefile: qcow1: Check for BACKING_STORE_OK storagefile: qcow1: Fix check for empty backing file storagefile: qcow1: Let qcowXGetBackingStore fill in format storagefile: Check version to determine if qcow2 or not storagefile: Drop now unused isQCow2 argument storagefile: Use qcowXGetBackingStore directly storagefile: Push 'start' into qcow2GetBackingStoreFormat storagefile: Push extension_end calc to qcow2GetBackingStoreFormat storagefile: Rename qcow2GetBackingStoreFormat storagefile: Rename qcow2GetExtensions 'format' argument storagefile: Fix backing format \0 check storagefile: Add externalDataStoreRaw member storagefile: Parse qcow2 external data file storagefile: Fill in meta->externalDataStoreRaw storagefile: Don't access backingStoreRaw directly in FromBackingRelative storagefile: Split out virStorageSourceNewFromChild storagefile: Add externalDataStore member storagefile: Fill in meta->externalDataStore security: dac: Drop !parent handling in SetImageLabelInternal security: dac: Add is_toplevel to SetImageLabelInternal security: dac: Restore image label for externalDataStore security: dac: break out SetImageLabelRelative security: dac: Label externalDataStore security: selinux: Simplify SetImageLabelInternal security: selinux: Drop !parent handling in SetImageLabelInternal security: selinux: Add is_toplevel to SetImageLabelInternal security: selinux: Restore image label for externalDataStore security: selinux: break out SetImageLabelRelative security: selinux: Label externalDataStore

Hi Cole, it seems the changes to dac/selinux follow a common pattern, in the past those changes then mostly applied to security-apparmor as well. Are you going to add patches for that security backend as well before this is final or do you expect the apparmor users Debian/Suse/Ubuntu to do so later on?

Furthermore for the static XML->Apparmor part it will most likely need an extension to [1] as well. Fortunately as you said it is very similar to backingDevices which means it should be rather easy to add this.

[1]: https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/security/virt-aa-helper....

I forgot about apparmor, sorry. I just sent a series and CCd you that does some apparmor cleanups and tweaks so that for this series the data_file coverage is just this extra diff: diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index d9f6b5638b..fc095d2964 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -948,6 +948,10 @@ storage_source_add_files(virStorageSourcePtr src, if (add_file_path(tmp, depth, buf) < 0) return -1; + if (src->externalDataStore && + storage_source_add_files(src->externalDataStore, buf, depth) < 0) + return -1; + depth++; } I will add a proper patch for that when the prep work hits git Thanks, Cole