4:49 p.m.
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
src/libvirt_private.syms | 1 -
src/security/security_dac.c | 63 +++++--
src/security/security_selinux.c | 97 +++++++----
src/util/virstoragefile.c | 281 ++++++++++++++++++++------------
src/util/virstoragefile.h | 11 +-
5 files changed, 290 insertions(+), 163 deletions(-)
--
2.23.0
4:49 p.m.
New subject: [libvirt] [PATCH 01/30] storagefile: Make GetMetadataInternal static
It is only used in virstoragefile.c
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/libvirt_private.syms | 1 -
src/util/virstoragefile.c | 2 +-
src/util/virstoragefile.h | 6 ------
3 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index eeab820eca..ca9581bd94 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2988,7 +2988,6 @@ virStorageFileGetLVMKey;
virStorageFileGetMetadata;
virStorageFileGetMetadataFromBuf;
virStorageFileGetMetadataFromFD;
-virStorageFileGetMetadataInternal;
virStorageFileGetNPIVKey;
virStorageFileGetRelativeBackingPath;
virStorageFileGetSCSIKey;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 3201f57e62..51726006e7 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -970,7 +970,7 @@ virStorageFileGetEncryptionPayloadOffset(const struct
FileEncryptionInfo *info,
* Note that this function may be called repeatedly on @meta, so it must
* clean up any existing allocated memory which would be overwritten.
*/
-int
+static int
virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
char *buf,
size_t len,
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 81b83a53ef..2472d89c85 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -346,12 +346,6 @@ struct _virStorageSource {
int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid);
-int virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
- char *buf,
- size_t len,
- int *backingFormat)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-
virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path,
int fd,
int format,
--
2.23.0
2:17 p.m.
New subject: [libvirt] [PATCH 01/30] storagefile: Make GetMetadataInternal static
On 10/7/19 6:49 PM, Cole Robinson wrote:
It is only used in virstoragefile.c
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/libvirt_private.syms | 1 -
src/util/virstoragefile.c | 2 +-
src/util/virstoragefile.h | 6 ------
3 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index eeab820eca..ca9581bd94 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2988,7 +2988,6 @@ virStorageFileGetLVMKey;
virStorageFileGetMetadata;
virStorageFileGetMetadataFromBuf;
virStorageFileGetMetadataFromFD;
-virStorageFileGetMetadataInternal;
virStorageFileGetNPIVKey;
virStorageFileGetRelativeBackingPath;
virStorageFileGetSCSIKey;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 3201f57e62..51726006e7 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -970,7 +970,7 @@ virStorageFileGetEncryptionPayloadOffset(const struct
FileEncryptionInfo *info,
* Note that this function may be called repeatedly on @meta, so it must
* clean up any existing allocated memory which would be overwritten.
*/
-int
+static int
virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
char *buf,
size_t len,
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 81b83a53ef..2472d89c85 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -346,12 +346,6 @@ struct _virStorageSource {
int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid);
-int virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
- char *buf,
- size_t len,
- int *backingFormat)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-
virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path,
int fd,
int format,
4:49 p.m.
New subject: [libvirt] [PATCH 02/30] storagefile: qcow1: Check for BACKING_STORE_OK
Check explicitly for BACKING_STORE_OK and not its 0 value
Signed-off-by: Cole Robinson <crobinso(a)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;
}
--
2.23.0
2:20 p.m.
New subject: [libvirt] [PATCH 02/30] storagefile: qcow1: Check for BACKING_STORE_OK
On 10/7/19 6:49 PM, Cole Robinson wrote:
Check explicitly for BACKING_STORE_OK and not its 0 value
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.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;
}
8:05 a.m.
New subject: [libvirt] [PATCH 02/30] storagefile: qcow1: Check for BACKING_STORE_OK
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(a)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
2:19 p.m.
New subject: [libvirt] [PATCH 02/30] storagefile: qcow1: Check for BACKING_STORE_OK
On 10/11/19 9:05 AM, Michal Privoznik wrote:
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(a)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.
FYI I've pushed the series now. I think after everything is applied
there isn't anything left to do here regarding your suggestion? But
correct me if I'm wrong
Thanks,
Cole
4:49 p.m.
New subject: [libvirt] [PATCH 03/30] storagefile: qcow1: Fix check for empty backing file
From f772b3d91fd the intention of this code seems to be to set
format=NONE when the image does not have a backing file. However
'buf' here is the whole qcow1 file header. What we want to be
checking is 'res' which is the parsed backing file path.
qcowXGetBackingStore sets this to NULL when there's no backing file.
Signed-off-by: Cole Robinson <crobinso(a)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 1549067c48..016c8f0799 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 == BACKING_STORE_OK && *buf == '\0')
+ if (ret == BACKING_STORE_OK && !*res)
*format = VIR_STORAGE_FILE_NONE;
return ret;
}
--
2.23.0
2:26 p.m.
New subject: [libvirt] [PATCH 03/30] storagefile: qcow1: Fix check for empty backing file
On 10/7/19 6:49 PM, Cole Robinson wrote:
>From f772b3d91fd the intention of this code seems to be to set
Is this leading '>' a bogus?
format=NONE when the image does not have a backing file. However
'buf' here is the whole qcow1 file header. What we want to be
checking is 'res' which is the parsed backing file path.
qcowXGetBackingStore sets this to NULL when there's no backing file.
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
This is unusual. Either the qcow file header 'buf' is being set to \0 when
there is no backing file (which would make the current code work even
when checking the wrong thing) or this is a bug that flew under the radar
from a long time.
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.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 1549067c48..016c8f0799 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 == BACKING_STORE_OK && *buf == '\0')
+ if (ret == BACKING_STORE_OK && !*res)
*format = VIR_STORAGE_FILE_NONE;
return ret;
}
8:05 a.m.
New subject: [libvirt] [PATCH 03/30] storagefile: qcow1: Fix check for empty backing file
On 10/9/19 9:26 PM, Daniel Henrique Barboza wrote:
On 10/7/19 6:49 PM, Cole Robinson wrote:
> >From f772b3d91fd the intention of this code seems to be to set
Is this leading '>' a bogus?
That's a feature of git-send-email so that this line is not mistaken for
e-mail header line "From: Michal Privoznik ....". When Cole pushes these
patches, it won't appear in git log, nor it shows in his local branch.
> format=NONE when the image does not have a backing file. However
> 'buf' here is the whole qcow1 file header. What we want to be
> checking is 'res' which is the parsed backing file path.
> qcowXGetBackingStore sets this to NULL when there's no backing file.
>
> Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
> ---
This is unusual. Either the qcow file header 'buf' is being set to \0 when
there is no backing file (which would make the current code work even
when checking the wrong thing) or this is a bug that flew under the radar
from a long time.
Since this is for qcow1 I wouldn't be surprised if it is the latter one.
I mean, does anybody even use qcow1 these days?
Michal
4:49 p.m.
New subject: [libvirt] [PATCH 04/30] storagefile: qcow1: Let qcowXGetBackingStore fill in format
Letting qcowXGetBackingStore fill in format gives the same behavior
we were opencoding in qcow1GetBackingStore
Signed-off-by: Cole Robinson <crobinso(a)redhat.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
--
2.23.0
3:43 p.m.
New subject: [libvirt] [PATCH 04/30] storagefile: qcow1: Let qcowXGetBackingStore fill in format
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(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)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
4:49 p.m.
New subject: [libvirt] [PATCH 05/30] storagefile: Check version to determine if qcow2 or not
Rather than require a boolean to be passed in
Signed-off-by: Cole Robinson <crobinso(a)redhat.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);
}
--
2.23.0
3:50 p.m.
New subject: [libvirt] [PATCH 05/30] storagefile: Check version to determine if qcow2 or not
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(a)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(a)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);
}
4:49 p.m.
New subject: [libvirt] [PATCH 06/30] storagefile: Drop now unused isQCow2 argument
Signed-off-by: Cole Robinson <crobinso(a)redhat.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);
}
--
2.23.0
3:52 p.m.
New subject: [libvirt] [PATCH 06/30] storagefile: Drop now unused isQCow2 argument
On 10/7/19 6:49 PM, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)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);
}
4:49 p.m.
New subject: [libvirt] [PATCH 07/30] storagefile: Use qcowXGetBackingStore directly
The qcow1 and qcow2 variants are identical, so remove the wrappers
Signed-off-by: Cole Robinson <crobinso(a)redhat.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,
--
2.23.0
3:55 p.m.
New subject: [libvirt] [PATCH 07/30] storagefile: Use qcowXGetBackingStore directly
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(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)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,
4:49 p.m.
New subject: [libvirt] [PATCH 08/30] storagefile: Push 'start' into qcow2GetBackingStoreFormat
This is a step towards making this qcow2GetBackingStoreFormat into
a generic qcow2 extensions parser
Signed-off-by: Cole Robinson <crobinso(a)redhat.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;
}
--
2.23.0
4 p.m.
New subject: [libvirt] [PATCH 08/30] storagefile: Push 'start' into qcow2GetBackingStoreFormat
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(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)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;
}
4:49 p.m.
New subject: [libvirt] [PATCH 09/30] storagefile: Push extension_end calc to qcow2GetBackingStoreFormat
This is a step towards making this qcow2GetBackingStoreFormat into
a generic qcow2 extensions parser
Signed-off-by: Cole Robinson <crobinso(a)redhat.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;
--
2.23.0
4:02 p.m.
New subject: [libvirt] [PATCH 09/30] storagefile: Push extension_end calc to qcow2GetBackingStoreFormat
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(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)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;
4:49 p.m.
New subject: [libvirt] [PATCH 10/30] storagefile: Rename qcow2GetBackingStoreFormat
...to qcow2GetExtensions. We will extend it for more extension
parsing in future patches
Signed-off-by: Cole Robinson <crobinso(a)redhat.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;
--
2.23.0
4:04 p.m.
New subject: [libvirt] [PATCH 10/30] storagefile: Rename qcow2GetBackingStoreFormat
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(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)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;
4:49 p.m.
New subject: [libvirt] [PATCH 11/30] storagefile: Rename qcow2GetExtensions 'format' argument
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(a)redhat.com>
---
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;
return BACKING_STORE_OK;
--
2.23.0
4:14 p.m.
New subject: [libvirt] [PATCH 11/30] storagefile: Rename qcow2GetExtensions 'format' argument
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(a)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(a)gmail.com>
return BACKING_STORE_OK;
12:47 p.m.
New subject: [libvirt] [PATCH 11/30] storagefile: Rename qcow2GetExtensions 'format' argument
On 10/9/19 5:14 PM, Daniel Henrique Barboza wrote:
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(a)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.
Later patches don't change it, as you've seen. We could change it, but
context wise it's less necessary there IMO, and also we should then
change it for the other GetBackingStore functions as well.
Thanks,
Cole
4:49 p.m.
New subject: [libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check
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(a)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;
}
+ }
offset += len;
}
--
2.23.0
4:34 p.m.
New subject: [libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check
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(a)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.
DHB
offset += len;
}
12:54 p.m.
New subject: [libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check
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(a)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
4:29 a.m.
New subject: [libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check
Daniel Henrique Barboza writes:
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?
No, that is the way email "protects" lines that begin with "From".
You
rarely see it nowadays because the mail itself is encoded, but if you
use plain text and send in ASCII, this will happen.
See page 78 of "The Unix Haters Handbook" [1] for details
[1] https://web.mit.edu/~simsong/www/ugh.pdf
--
Cheers,
Christophe de Dinechin (IRC c3d)
8:04 a.m.
New subject: [libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check
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(a)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
1:03 p.m.
New subject: [libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check
On 10/11/19 9:04 AM, Michal Privoznik wrote:
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(a)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.
Thanks, will fix before pushing
- Cole
4:49 p.m.
New subject: [libvirt] [PATCH 13/30] storagefile: Add externalDataStoreRaw member
Add the plumbing to track a qcow2 external data file path in
virStorageSource
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/util/virstoragefile.c | 2 ++
src/util/virstoragefile.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 5a4e4b24ae..621cc56e87 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2258,6 +2258,7 @@ virStorageSourceCopy(const virStorageSource *src,
VIR_STRDUP(def->volume, src->volume) < 0 ||
VIR_STRDUP(def->relPath, src->relPath) < 0 ||
VIR_STRDUP(def->backingStoreRaw, src->backingStoreRaw) < 0 ||
+ VIR_STRDUP(def->externalDataStoreRaw, src->externalDataStoreRaw) < 0 ||
VIR_STRDUP(def->snapshot, src->snapshot) < 0 ||
VIR_STRDUP(def->configFile, src->configFile) < 0 ||
VIR_STRDUP(def->nodeformat, src->nodeformat) < 0 ||
@@ -2533,6 +2534,7 @@ virStorageSourceClear(virStorageSourcePtr def)
virStorageSourceSeclabelsClear(def);
virStoragePermsFree(def->perms);
VIR_FREE(def->timestamps);
+ VIR_FREE(def->externalDataStoreRaw);
virStorageNetHostDefFree(def->nhosts, def->hosts);
virStorageAuthDefFree(def->auth);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 2472d89c85..bbff511657 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -302,6 +302,8 @@ struct _virStorageSource {
/* Name of the child backing store recorded in metadata of the
* current file. */
char *backingStoreRaw;
+ /* Name of the child data file recorded in metadata of the current file. */
+ char *externalDataStoreRaw;
/* metadata that allows identifying given storage source */
char *nodeformat; /* name of the format handler object */
--
2.23.0
4:38 p.m.
New subject: [libvirt] [PATCH 13/30] storagefile: Add externalDataStoreRaw member
On 10/7/19 6:49 PM, Cole Robinson wrote:
Add the plumbing to track a qcow2 external data file path in
virStorageSource
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/util/virstoragefile.c | 2 ++
src/util/virstoragefile.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 5a4e4b24ae..621cc56e87 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2258,6 +2258,7 @@ virStorageSourceCopy(const virStorageSource *src,
VIR_STRDUP(def->volume, src->volume) < 0 ||
VIR_STRDUP(def->relPath, src->relPath) < 0 ||
VIR_STRDUP(def->backingStoreRaw, src->backingStoreRaw) < 0 ||
+ VIR_STRDUP(def->externalDataStoreRaw, src->externalDataStoreRaw) < 0
||
VIR_STRDUP(def->snapshot, src->snapshot) < 0 ||
VIR_STRDUP(def->configFile, src->configFile) < 0 ||
VIR_STRDUP(def->nodeformat, src->nodeformat) < 0 ||
@@ -2533,6 +2534,7 @@ virStorageSourceClear(virStorageSourcePtr def)
virStorageSourceSeclabelsClear(def);
virStoragePermsFree(def->perms);
VIR_FREE(def->timestamps);
+ VIR_FREE(def->externalDataStoreRaw);
virStorageNetHostDefFree(def->nhosts, def->hosts);
virStorageAuthDefFree(def->auth);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 2472d89c85..bbff511657 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -302,6 +302,8 @@ struct _virStorageSource {
/* Name of the child backing store recorded in metadata of the
* current file. */
char *backingStoreRaw;
+ /* Name of the child data file recorded in metadata of the current file. */
+ char *externalDataStoreRaw;
/* metadata that allows identifying given storage source */
char *nodeformat; /* name of the format handler object */
4:49 p.m.
New subject: [libvirt] [PATCH 14/30] storagefile: Parse qcow2 external data file
This is tracked as a qcow2 extension, like backing store format
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/util/virstoragefile.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 621cc56e87..7e32d7619e 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -202,6 +202,7 @@ qedGetBackingStore(char **, int *, const char *, size_t);
#define QCOW2_HDR_EXTENSION_END 0
#define QCOW2_HDR_EXTENSION_BACKING_FORMAT 0xE2792ACA
+#define QCOW2_HDR_EXTENSION_DATA_FILE 0x44415441
#define QCOW2v3_HDR_FEATURES_INCOMPATIBLE (QCOW2_HDR_TOTAL_SIZE)
#define QCOW2v3_HDR_FEATURES_COMPATIBLE (QCOW2v3_HDR_FEATURES_INCOMPATIBLE+8)
@@ -429,7 +430,8 @@ cowGetBackingStore(char **res,
static int
qcow2GetExtensions(const char *buf,
size_t buf_size,
- int *backingFormat)
+ int *backingFormat,
+ char **externalDataStoreRaw)
{
size_t offset;
size_t extension_start;
@@ -508,6 +510,9 @@ qcow2GetExtensions(const char *buf,
case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {
VIR_AUTOFREE(char *) tmp = NULL;
+ if (!backingFormat)
+ break;
+
if (VIR_ALLOC_N(tmp, len + 1) < 0)
return -1;
memcpy(tmp, buf + offset, len);
@@ -516,6 +521,19 @@ qcow2GetExtensions(const char *buf,
*backingFormat = virStorageFileFormatTypeFromString(tmp);
if (*backingFormat <= VIR_STORAGE_FILE_NONE)
return -1;
+ break;
+ }
+
+ case QCOW2_HDR_EXTENSION_DATA_FILE: {
+ if (!externalDataStoreRaw)
+ break;
+
+ if (VIR_ALLOC_N(*externalDataStoreRaw, len + 1) < 0)
+ return -1;
+ memcpy(*externalDataStoreRaw, buf + offset, len);
+ (*externalDataStoreRaw)[len] = '\0';
+ VIR_DEBUG("parsed externalDataStoreRaw='%s'",
*externalDataStoreRaw);
+ break;
}
}
@@ -566,7 +584,7 @@ qcowXGetBackingStore(char **res,
memcpy(*res, buf + offset, size);
(*res)[size] = '\0';
- if (qcow2GetExtensions(buf, buf_size, format) < 0)
+ if (qcow2GetExtensions(buf, buf_size, format, NULL) < 0)
return BACKING_STORE_INVALID;
return BACKING_STORE_OK;
--
2.23.0
4:47 p.m.
New subject: [libvirt] [PATCH 14/30] storagefile: Parse qcow2 external data file
On 10/7/19 6:49 PM, Cole Robinson wrote:
This is tracked as a qcow2 extension, like backing store format
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/util/virstoragefile.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 621cc56e87..7e32d7619e 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -202,6 +202,7 @@ qedGetBackingStore(char **, int *, const char *, size_t);
#define QCOW2_HDR_EXTENSION_END 0
#define QCOW2_HDR_EXTENSION_BACKING_FORMAT 0xE2792ACA
+#define QCOW2_HDR_EXTENSION_DATA_FILE 0x44415441
#define QCOW2v3_HDR_FEATURES_INCOMPATIBLE (QCOW2_HDR_TOTAL_SIZE)
#define QCOW2v3_HDR_FEATURES_COMPATIBLE (QCOW2v3_HDR_FEATURES_INCOMPATIBLE+8)
@@ -429,7 +430,8 @@ cowGetBackingStore(char **res,
static int
qcow2GetExtensions(const char *buf,
size_t buf_size,
- int *backingFormat)
+ int *backingFormat,
+ char **externalDataStoreRaw)
{
size_t offset;
size_t extension_start;
@@ -508,6 +510,9 @@ qcow2GetExtensions(const char *buf,
case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {
VIR_AUTOFREE(char *) tmp = NULL;
+ if (!backingFormat)
+ break;
+
if (VIR_ALLOC_N(tmp, len + 1) < 0)
return -1;
memcpy(tmp, buf + offset, len);
@@ -516,6 +521,19 @@ qcow2GetExtensions(const char *buf,
*backingFormat = virStorageFileFormatTypeFromString(tmp);
if (*backingFormat <= VIR_STORAGE_FILE_NONE)
return -1;
+ break;
+ }
+
+ case QCOW2_HDR_EXTENSION_DATA_FILE: {
+ if (!externalDataStoreRaw)
+ break;
+
+ if (VIR_ALLOC_N(*externalDataStoreRaw, len + 1) < 0)
+ return -1;
+ memcpy(*externalDataStoreRaw, buf + offset, len);
+ (*externalDataStoreRaw)[len] = '\0';
+ VIR_DEBUG("parsed externalDataStoreRaw='%s'",
*externalDataStoreRaw);
+ break;
}
}
@@ -566,7 +584,7 @@ qcowXGetBackingStore(char **res,
memcpy(*res, buf + offset, size);
(*res)[size] = '\0';
- if (qcow2GetExtensions(buf, buf_size, format) < 0)
+ if (qcow2GetExtensions(buf, buf_size, format, NULL) < 0)
return BACKING_STORE_INVALID;
return BACKING_STORE_OK;
4:49 p.m.
New subject: [libvirt] [PATCH 15/30] storagefile: Fill in meta->externalDataStoreRaw
Call qcow2GetExtensions to actually fill in the virStorageSource
externalDataStoreRaw member
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/util/virstoragefile.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7e32d7619e..53fe4590b4 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1060,6 +1060,12 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
fileTypeInfo[meta->format].getFeatures(&meta->features,
meta->format, buf, len) < 0)
return -1;
+ VIR_FREE(meta->externalDataStoreRaw);
+ if (meta->format == VIR_STORAGE_FILE_QCOW2 &&
+ qcow2GetExtensions(buf, len, NULL, &meta->externalDataStoreRaw) < 0) {
+ return -1;
+ }
+
VIR_FREE(meta->compat);
if (meta->format == VIR_STORAGE_FILE_QCOW2 && meta->features
&&
VIR_STRDUP(meta->compat, "1.1") < 0)
--
2.23.0
4:54 p.m.
New subject: [libvirt] [PATCH 15/30] storagefile: Fill in meta->externalDataStoreRaw
On 10/7/19 6:49 PM, Cole Robinson wrote:
Call qcow2GetExtensions to actually fill in the virStorageSource
externalDataStoreRaw member
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/util/virstoragefile.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7e32d7619e..53fe4590b4 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1060,6 +1060,12 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
fileTypeInfo[meta->format].getFeatures(&meta->features,
meta->format, buf, len) < 0)
return -1;
+ VIR_FREE(meta->externalDataStoreRaw);
+ if (meta->format == VIR_STORAGE_FILE_QCOW2 &&
+ qcow2GetExtensions(buf, len, NULL, &meta->externalDataStoreRaw) < 0)
{
+ return -1;
+ }
+
VIR_FREE(meta->compat);
if (meta->format == VIR_STORAGE_FILE_QCOW2 && meta->features
&&
VIR_STRDUP(meta->compat, "1.1") < 0)
4:49 p.m.
New subject: [libvirt] [PATCH 16/30] storagefile: Don't access backingStoreRaw directly in FromBackingRelative
For the only usage, the rel == parent->backingStoreRaw, so drop
the direct access
Signed-off-by: Cole Robinson <crobinso(a)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 53fe4590b4..c47df6c200 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2625,7 +2625,7 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent,
return NULL;
/* store relative name */
- if (VIR_STRDUP(def->relPath, parent->backingStoreRaw) < 0)
+ if (VIR_STRDUP(def->relPath, rel) < 0)
return NULL;
if (!(dirname = mdir_name(parent->path))) {
--
2.23.0
4:59 p.m.
New subject: [libvirt] [PATCH 16/30] storagefile: Don't access backingStoreRaw directly in FromBackingRelative
On 10/7/19 6:49 PM, Cole Robinson wrote:
For the only usage, the rel == parent->backingStoreRaw, so drop
the direct access
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.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 53fe4590b4..c47df6c200 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2625,7 +2625,7 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent,
return NULL;
/* store relative name */
- if (VIR_STRDUP(def->relPath, parent->backingStoreRaw) < 0)
+ if (VIR_STRDUP(def->relPath, rel) < 0)
return NULL;
if (!(dirname = mdir_name(parent->path))) {
4:49 p.m.
New subject: [libvirt] [PATCH 17/30] storagefile: Split out virStorageSourceNewFromChild
Future patches will use this for external data file handling
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/util/virstoragefile.c | 47 ++++++++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index c47df6c200..7ae6719dd6 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3716,38 +3716,38 @@ virStorageSourceNewFromBackingAbsolute(const char *path,
/**
- * virStorageSourceNewFromBacking:
+ * virStorageSourceNewFromChild:
* @parent: storage source parent
- * @backing: returned backing store definition
+ * @child: returned child/backing store definition
+ * @parentRaw: raw child string (backingStoreRaw)
*
* Creates a storage source which describes the backing image of @parent and
- * fills it into @backing depending on the 'backingStoreRaw' property of @parent
+ * fills it into @backing depending on the passed parentRaw (backingStoreRaw)
* and other data. Note that for local storage this function accesses the file
- * to update the actual type of the backing store.
+ * to update the actual type of the child store.
*
- * Returns 0 on success, 1 if we could parse all location data but the backinig
+ * Returns 0 on success, 1 if we could parse all location data but the child
* store specification contained other data unrepresentable by libvirt (e.g.
* inline authentication).
* In both cases @src is filled. On error -1 is returned @src is NULL and an
* error is reported.
*/
-int
-virStorageSourceNewFromBacking(virStorageSourcePtr parent,
- virStorageSourcePtr *backing)
+static int
+virStorageSourceNewFromChild(virStorageSourcePtr parent,
+ const char *parentRaw,
+ virStorageSourcePtr *child)
{
struct stat st;
VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
int rc = 0;
- *backing = NULL;
+ *child = NULL;
- if (virStorageIsRelative(parent->backingStoreRaw)) {
- if (!(def = virStorageSourceNewFromBackingRelative(parent,
- parent->backingStoreRaw)))
+ if (virStorageIsRelative(parentRaw)) {
+ if (!(def = virStorageSourceNewFromBackingRelative(parent, parentRaw)))
return -1;
} else {
- if ((rc = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw,
- &def)) < 0)
+ if ((rc = virStorageSourceNewFromBackingAbsolute(parentRaw, &def)) < 0)
return -1;
}
@@ -3767,10 +3767,25 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent,
if (virStorageSourceInitChainElement(def, parent, true) < 0)
return -1;
- def->readonly = true;
def->detected = true;
- VIR_STEAL_PTR(*backing, def);
+ VIR_STEAL_PTR(*child, def);
+ return rc;
+}
+
+
+int
+virStorageSourceNewFromBacking(virStorageSourcePtr parent,
+ virStorageSourcePtr *backing)
+{
+ int rc;
+
+ if ((rc = virStorageSourceNewFromChild(parent,
+ parent->backingStoreRaw,
+ backing)) < 0)
+ return rc;
+
+ (*backing)->readonly = true;
return rc;
}
--
2.23.0
9:40 a.m.
New subject: [libvirt] [PATCH 17/30] storagefile: Split out virStorageSourceNewFromChild
On 10/7/19 6:49 PM, Cole Robinson wrote:
Future patches will use this for external data file handling
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/util/virstoragefile.c | 47
++++++++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index c47df6c200..7ae6719dd6 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3716,38 +3716,38 @@ virStorageSourceNewFromBackingAbsolute(const char *path,
/**
- * virStorageSourceNewFromBacking:
+ * virStorageSourceNewFromChild:
* @parent: storage source parent
- * @backing: returned backing store definition
+ * @child: returned child/backing store definition
+ * @parentRaw: raw child string (backingStoreRaw)
*
* Creates a storage source which describes the backing image of @parent and
- * fills it into @backing depending on the 'backingStoreRaw' property of
@parent
+ * fills it into @backing depending on the passed parentRaw (backingStoreRaw)
* and other data. Note that for local storage this function accesses the file
- * to update the actual type of the backing store.
+ * to update the actual type of the child store.
*
- * Returns 0 on success, 1 if we could parse all location data but the backinig
+ * Returns 0 on success, 1 if we could parse all location data but the child
* store specification contained other data unrepresentable by libvirt (e.g.
* inline authentication).
* In both cases @src is filled. On error -1 is returned @src is NULL and an
* error is reported.
*/
-int
-virStorageSourceNewFromBacking(virStorageSourcePtr parent,
- virStorageSourcePtr *backing)
+static int
+virStorageSourceNewFromChild(virStorageSourcePtr parent,
+ const char *parentRaw,
+ virStorageSourcePtr *child)
{
struct stat st;
VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
int rc = 0;
- *backing = NULL;
+ *child = NULL;
- if (virStorageIsRelative(parent->backingStoreRaw)) {
- if (!(def = virStorageSourceNewFromBackingRelative(parent,
-
parent->backingStoreRaw)))
+ if (virStorageIsRelative(parentRaw)) {
+ if (!(def = virStorageSourceNewFromBackingRelative(parent, parentRaw)))
return -1;
} else {
- if ((rc = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw,
- &def)) < 0)
+ if ((rc = virStorageSourceNewFromBackingAbsolute(parentRaw, &def)) < 0)
return -1;
}
@@ -3767,10 +3767,25 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent,
if (virStorageSourceInitChainElement(def, parent, true) < 0)
return -1;
- def->readonly = true;
def->detected = true;
- VIR_STEAL_PTR(*backing, def);
+ VIR_STEAL_PTR(*child, def);
+ return rc;
+}
+
+
+int
+virStorageSourceNewFromBacking(virStorageSourcePtr parent,
+ virStorageSourcePtr *backing)
+{
+ int rc;
+
+ if ((rc = virStorageSourceNewFromChild(parent,
+ parent->backingStoreRaw,
+ backing)) < 0)
+ return rc;
+
+ (*backing)->readonly = true;
return rc;
}
4:49 p.m.
New subject: [libvirt] [PATCH 18/30] storagefile: Add externalDataStore member
Add the plumbing to track a externalDataStoreRaw as a virStorageSource
Signed-off-by: Cole Robinson <crobinso(a)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;
+
virStorageNetHostDefFree(def->nhosts, def->hosts);
virStorageAuthDefFree(def->auth);
virObjectUnref(def->privateData);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index bbff511657..d84dad052d 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -292,6 +292,9 @@ struct _virStorageSource {
/* backing chain of the storage source */
virStorageSourcePtr backingStore;
+ /* external data store storage source */
+ virStorageSourcePtr externalDataStore;
+
/* metadata for storage driver access to remote and local volumes */
virStorageDriverDataPtr drv;
--
2.23.0
10:09 a.m.
New subject: [libvirt] [PATCH 18/30] storagefile: Add externalDataStore member
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(a)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).
LGTM otherwise
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
+
virStorageNetHostDefFree(def->nhosts, def->hosts);
virStorageAuthDefFree(def->auth);
virObjectUnref(def->privateData);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index bbff511657..d84dad052d 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -292,6 +292,9 @@ struct _virStorageSource {
/* backing chain of the storage source */
virStorageSourcePtr backingStore;
+ /* external data store storage source */
+ virStorageSourcePtr externalDataStore;
+
/* metadata for storage driver access to remote and local volumes */
virStorageDriverDataPtr drv;
8:04 a.m.
New subject: [libvirt] [PATCH 18/30] storagefile: Add externalDataStore member
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(a)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
9:27 a.m.
New subject: [libvirt] [PATCH 18/30] storagefile: Add externalDataStore member
On 10/11/19 10:04 AM, Michal Privoznik wrote:
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(a)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.
Got it. I didn't get across an example of this re-use yet, didn't know
it is a
valid possibility.
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
I was right in the end! That all I'm going to remember ;)
DHB
Michal
4:49 p.m.
New subject: [libvirt] [PATCH 19/30] storagefile: Fill in meta->externalDataStore
Add virStorageSourceNewFromExternalData, similar to
virStorageSourceNewFromBacking and use it to fill in a
virStorageSource for externalDataStore
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/util/virstoragefile.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index ce669b6e0b..4aa70d71b1 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3799,6 +3799,24 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent,
}
+static int
+virStorageSourceNewFromExternalData(virStorageSourcePtr parent,
+ virStorageSourcePtr *externalDataStore)
+{
+ int rc;
+
+ if ((rc = virStorageSourceNewFromChild(parent,
+ parent->externalDataStoreRaw,
+ externalDataStore)) < 0)
+ return rc;
+
+ /* qcow2 data_file can only be raw */
+ (*externalDataStore)->format = VIR_STORAGE_FILE_RAW;
+ (*externalDataStore)->readonly = parent->readonly;
+ return rc;
+}
+
+
/**
* @src: disk source definition structure
* @fd: file descriptor
@@ -5007,6 +5025,23 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
}
VIR_STEAL_PTR(src->backingStore, backingStore);
+
+ if (src->externalDataStoreRaw) {
+ VIR_AUTOUNREF(virStorageSourcePtr) externalDataStore = NULL;
+
+ if ((rv = virStorageSourceNewFromExternalData(src,
+ &externalDataStore)) < 0)
+ goto cleanup;
+
+ if (rv == 1) {
+ /* the file would not be usable for VM usage */
+ ret = 0;
+ goto cleanup;
+ }
+
+ VIR_STEAL_PTR(src->externalDataStore, externalDataStore);
+ }
+
ret = 0;
cleanup:
--
2.23.0
10:14 a.m.
New subject: [libvirt] [PATCH 19/30] storagefile: Fill in meta->externalDataStore
On 10/7/19 6:49 PM, Cole Robinson wrote:
Add virStorageSourceNewFromExternalData, similar to
virStorageSourceNewFromBacking and use it to fill in a
virStorageSource for externalDataStore
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/util/virstoragefile.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index ce669b6e0b..4aa70d71b1 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3799,6 +3799,24 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent,
}
+static int
+virStorageSourceNewFromExternalData(virStorageSourcePtr parent,
+ virStorageSourcePtr *externalDataStore)
+{
+ int rc;
+
+ if ((rc = virStorageSourceNewFromChild(parent,
+ parent->externalDataStoreRaw,
+ externalDataStore)) < 0)
+ return rc;
+
+ /* qcow2 data_file can only be raw */
+ (*externalDataStore)->format = VIR_STORAGE_FILE_RAW;
+ (*externalDataStore)->readonly = parent->readonly;
+ return rc;
+}
+
+
/**
* @src: disk source definition structure
* @fd: file descriptor
@@ -5007,6 +5025,23 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
}
VIR_STEAL_PTR(src->backingStore, backingStore);
+
+ if (src->externalDataStoreRaw) {
+ VIR_AUTOUNREF(virStorageSourcePtr) externalDataStore = NULL;
+
+ if ((rv = virStorageSourceNewFromExternalData(src,
+ &externalDataStore)) < 0)
+ goto cleanup;
+
+ if (rv == 1) {
+ /* the file would not be usable for VM usage */
+ ret = 0;
+ goto cleanup;
+ }
+
+ VIR_STEAL_PTR(src->externalDataStore, externalDataStore);
+ }
+
ret = 0;
cleanup:
4:49 p.m.
New subject: [libvirt] [PATCH 20/30] security: dac: Drop !parent handling in SetImageLabelInternal
The only caller always passes in a non-null parent
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/security/security_dac.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 4b4afef18a..bcc781213e 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -893,9 +893,8 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
return 0;
disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_DAC_NAME);
- if (parent)
- parent_seclabel = virStorageSourceGetSecurityLabelDef(parent,
- SECURITY_DAC_NAME);
+ parent_seclabel = virStorageSourceGetSecurityLabelDef(parent,
+ SECURITY_DAC_NAME);
if (disk_seclabel && (!disk_seclabel->relabel || disk_seclabel->label))
{
if (!disk_seclabel->relabel)
--
2.23.0
10:31 a.m.
New subject: [libvirt] [PATCH 20/30] security: dac: Drop !parent handling in SetImageLabelInternal
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(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/security/security_dac.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 4b4afef18a..bcc781213e 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -893,9 +893,8 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
return 0;
disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_DAC_NAME);
- if (parent)
- parent_seclabel = virStorageSourceGetSecurityLabelDef(parent,
- SECURITY_DAC_NAME);
+ parent_seclabel = virStorageSourceGetSecurityLabelDef(parent,
+ SECURITY_DAC_NAME);
if (disk_seclabel && (!disk_seclabel->relabel ||
disk_seclabel->label)) {
if (!disk_seclabel->relabel)
4:49 p.m.
New subject: [libvirt] [PATCH 21/30] security: dac: Add is_toplevel to SetImageLabelInternal
This will simplify future patches and make the logic easier to follow
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/security/security_dac.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index bcc781213e..6d0c8a9b1c 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -882,6 +882,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
bool remember;
+ bool is_toplevel = parent == src;
uid_t user;
gid_t group;
@@ -926,7 +927,7 @@ virSecurityDACSetImageLabelInternal(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;
return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember);
}
--
2.23.0
10:34 a.m.
New subject: [libvirt] [PATCH 21/30] security: dac: Add is_toplevel to SetImageLabelInternal
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(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/security/security_dac.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index bcc781213e..6d0c8a9b1c 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -882,6 +882,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
bool remember;
+ bool is_toplevel = parent == src;
uid_t user;
gid_t group;
@@ -926,7 +927,7 @@ virSecurityDACSetImageLabelInternal(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;
return virSecurityDACSetOwnership(mgr, src, NULL, user, group, remember);
}
4:49 p.m.
New subject: [libvirt] [PATCH 22/30] security: dac: Restore image label for externalDataStore
Rename the existing virSecurityDACRestoreImageLabelInt
to virSecurityDACRestoreImageLabelSingle, and extend the new
ImageLabelInt handle externalDataStore
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/security/security_dac.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 6d0c8a9b1c..befa388791 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -954,10 +954,10 @@ virSecurityDACSetImageLabel(virSecurityManagerPtr mgr,
static int
-virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virStorageSourcePtr src,
- bool migrated)
+virSecurityDACRestoreImageLabelSingle(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virStorageSourcePtr src,
+ bool migrated)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityLabelDefPtr secdef;
@@ -1008,6 +1008,26 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
}
+static int
+virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virStorageSourcePtr src,
+ bool migrated)
+{
+ if (virSecurityDACRestoreImageLabelSingle(mgr, def, src, migrated) < 0)
+ return -1;
+
+ if (src->externalDataStore &&
+ virSecurityDACRestoreImageLabelSingle(mgr,
+ def,
+ src->externalDataStore,
+ migrated) < 0)
+ return -1;
+
+ return 0;
+}
+
+
static int
virSecurityDACRestoreImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
--
2.23.0
10:56 a.m.
New subject: [libvirt] [PATCH 22/30] security: dac: Restore image label for externalDataStore
On 10/7/19 6:49 PM, Cole Robinson wrote:
Rename the existing virSecurityDACRestoreImageLabelInt
to virSecurityDACRestoreImageLabelSingle, and extend the new
ImageLabelInt handle externalDataStore
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/security/security_dac.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 6d0c8a9b1c..befa388791 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -954,10 +954,10 @@ virSecurityDACSetImageLabel(virSecurityManagerPtr mgr,
static int
-virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virStorageSourcePtr src,
- bool migrated)
+virSecurityDACRestoreImageLabelSingle(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virStorageSourcePtr src,
+ bool migrated)
{
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityLabelDefPtr secdef;
@@ -1008,6 +1008,26 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
}
+static int
+virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virStorageSourcePtr src,
+ bool migrated)
+{
+ if (virSecurityDACRestoreImageLabelSingle(mgr, def, src, migrated) < 0)
+ return -1;
+
+ if (src->externalDataStore &&
+ virSecurityDACRestoreImageLabelSingle(mgr,
+ def,
+ src->externalDataStore,
+ migrated) < 0)
+ return -1;
+
+ return 0;
+}
+
+
static int
virSecurityDACRestoreImageLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
4:49 p.m.
New subject: [libvirt] [PATCH 23/30] security: dac: break out SetImageLabelRelative
This will be used for recursing into externalDataStore
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/security/security_dac.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index befa388791..326b9b1a3c 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -934,15 +934,16 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
static int
-virSecurityDACSetImageLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virStorageSourcePtr src,
- virSecurityDomainImageLabelFlags flags)
+virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virStorageSourcePtr src,
+ virStorageSourcePtr parent,
+ virSecurityDomainImageLabelFlags flags)
{
virStorageSourcePtr n;
for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
- if (virSecurityDACSetImageLabelInternal(mgr, def, n, src) < 0)
+ if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent) < 0)
return -1;
if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))
@@ -952,6 +953,14 @@ virSecurityDACSetImageLabel(virSecurityManagerPtr mgr,
return 0;
}
+static int
+virSecurityDACSetImageLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virStorageSourcePtr src,
+ virSecurityDomainImageLabelFlags flags)
+{
+ return virSecurityDACSetImageLabelRelative(mgr, def, src, src, flags);
+}
static int
virSecurityDACRestoreImageLabelSingle(virSecurityManagerPtr mgr,
--
2.23.0
10:58 a.m.
New subject: [libvirt] [PATCH 23/30] security: dac: break out SetImageLabelRelative
On 10/7/19 6:49 PM, Cole Robinson wrote:
This will be used for recursing into externalDataStore
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/security/security_dac.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index befa388791..326b9b1a3c 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -934,15 +934,16 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
static int
-virSecurityDACSetImageLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- virStorageSourcePtr src,
- virSecurityDomainImageLabelFlags flags)
+virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virStorageSourcePtr src,
+ virStorageSourcePtr parent,
+ virSecurityDomainImageLabelFlags flags)
{
virStorageSourcePtr n;
for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
- if (virSecurityDACSetImageLabelInternal(mgr, def, n, src) < 0)
+ if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent) < 0)
return -1;
if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))
@@ -952,6 +953,14 @@ virSecurityDACSetImageLabel(virSecurityManagerPtr mgr,
return 0;
}
+static int
+virSecurityDACSetImageLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ virStorageSourcePtr src,
+ virSecurityDomainImageLabelFlags flags)
+{
+ return virSecurityDACSetImageLabelRelative(mgr, def, src, src, flags);
+}
static int
virSecurityDACRestoreImageLabelSingle(virSecurityManagerPtr mgr,
4:49 p.m.
New subject: [libvirt] [PATCH 24/30] security: dac: Label externalDataStore
We mirror the labeling strategy that was used for its sibling
image
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/security/security_dac.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 326b9b1a3c..2bbf773dd3 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -882,7 +882,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
bool remember;
- bool is_toplevel = parent == src;
+ bool is_toplevel = parent == src || parent->externalDataStore == src;
uid_t user;
gid_t group;
@@ -946,6 +946,14 @@ virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,
if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent) < 0)
return -1;
+ if (n->externalDataStore &&
+ virSecurityDACSetImageLabelRelative(mgr,
+ def,
+ n->externalDataStore,
+ parent,
+ flags) < 0)
+ return -1;
+
if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))
break;
}
--
2.23.0
10:59 a.m.
New subject: [libvirt] [PATCH 24/30] security: dac: Label externalDataStore
On 10/7/19 6:49 PM, Cole Robinson wrote:
We mirror the labeling strategy that was used for its sibling
image
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/security/security_dac.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 326b9b1a3c..2bbf773dd3 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -882,7 +882,7 @@ virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
bool remember;
- bool is_toplevel = parent == src;
+ bool is_toplevel = parent == src || parent->externalDataStore == src;
uid_t user;
gid_t group;
@@ -946,6 +946,14 @@ virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,
if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent) < 0)
return -1;
+ if (n->externalDataStore &&
+ virSecurityDACSetImageLabelRelative(mgr,
+ def,
+ n->externalDataStore,
+ parent,
+ flags) < 0)
+ return -1;
+
if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN))
break;
}
4:49 p.m.
New subject: [libvirt] [PATCH 25/30] security: selinux: Simplify SetImageLabelInternal
All the SetFileCon calls only differ by the label they pass in.
Rework the conditionals to track what label we need, and use a
single SetFileCon call
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/security/security_selinux.c | 31 ++++++++++---------------------
1 file changed, 10 insertions(+), 21 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index e879fa39ab..9d28bc5773 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1822,6 +1822,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
virSecurityLabelDefPtr secdef;
virSecurityDeviceLabelDefPtr disk_seclabel;
virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
+ char *use_label = NULL;
bool remember;
int ret;
@@ -1856,40 +1857,28 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr
mgr,
if (!disk_seclabel->relabel)
return 0;
- ret = virSecuritySELinuxSetFilecon(mgr, src->path,
- disk_seclabel->label, remember);
+ use_label = disk_seclabel->label;
} else if (parent_seclabel && (!parent_seclabel->relabel ||
parent_seclabel->label)) {
if (!parent_seclabel->relabel)
return 0;
- ret = virSecuritySELinuxSetFilecon(mgr, src->path,
- parent_seclabel->label, remember);
+ use_label = parent_seclabel->label;
} else if (!parent || parent == src) {
if (src->shared) {
- ret = virSecuritySELinuxSetFilecon(mgr,
- src->path,
- data->file_context,
- remember);
+ use_label = data->file_context;
} else if (src->readonly) {
- ret = virSecuritySELinuxSetFilecon(mgr,
- src->path,
- data->content_context,
- remember);
+ use_label = data->content_context;
} else if (secdef->imagelabel) {
- ret = virSecuritySELinuxSetFilecon(mgr,
- src->path,
- secdef->imagelabel,
- remember);
+ use_label = secdef->imagelabel;
} else {
- ret = 0;
+ return 0;
}
} else {
- ret = virSecuritySELinuxSetFilecon(mgr,
- src->path,
- data->content_context,
- remember);
+ use_label = data->content_context;
}
+ ret = virSecuritySELinuxSetFilecon(mgr, src->path, use_label, remember);
+
if (ret == 1 && !disk_seclabel) {
/* If we failed to set a label, but virt_use_nfs let us
* proceed anyway, then we don't need to relabel later. */
--
2.23.0
11:02 a.m.
New subject: [libvirt] [PATCH 25/30] security: selinux: Simplify SetImageLabelInternal
On 10/7/19 6:49 PM, Cole Robinson wrote:
All the SetFileCon calls only differ by the label they pass in.
Rework the conditionals to track what label we need, and use a
single SetFileCon call
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/security/security_selinux.c | 31
++++++++++---------------------
1 file changed, 10 insertions(+), 21 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index e879fa39ab..9d28bc5773 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1822,6 +1822,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
virSecurityLabelDefPtr secdef;
virSecurityDeviceLabelDefPtr disk_seclabel;
virSecurityDeviceLabelDefPtr parent_seclabel = NULL;
+ char *use_label = NULL;
bool remember;
int ret;
@@ -1856,40 +1857,28 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr
mgr,
if (!disk_seclabel->relabel)
return 0;
- ret = virSecuritySELinuxSetFilecon(mgr, src->path,
- disk_seclabel->label, remember);
+ use_label = disk_seclabel->label;
} else if (parent_seclabel && (!parent_seclabel->relabel ||
parent_seclabel->label)) {
if (!parent_seclabel->relabel)
return 0;
- ret = virSecuritySELinuxSetFilecon(mgr, src->path,
- parent_seclabel->label, remember);
+ use_label = parent_seclabel->label;
} else if (!parent || parent == src) {
if (src->shared) {
- ret = virSecuritySELinuxSetFilecon(mgr,
- src->path,
- data->file_context,
- remember);
+ use_label = data->file_context;
} else if (src->readonly) {
- ret = virSecuritySELinuxSetFilecon(mgr,
- src->path,
- data->content_context,
- remember);
+ use_label = data->content_context;
} else if (secdef->imagelabel) {
- ret = virSecuritySELinuxSetFilecon(mgr,
- src->path,
- secdef->imagelabel,
- remember);
+ use_label = secdef->imagelabel;
} else {
- ret = 0;
+ return 0;
}
} else {
- ret = virSecuritySELinuxSetFilecon(mgr,
- src->path,
- data->content_context,
- remember);
+ use_label = data->content_context;
}
+ ret = virSecuritySELinuxSetFilecon(mgr, src->path, use_label, remember);
+
if (ret == 1 && !disk_seclabel) {
/* If we failed to set a label, but virt_use_nfs let us
* proceed anyway, then we don't need to relabel later. */
8:04 a.m.
New subject: [libvirt] [PATCH 25/30] security: selinux: Simplify SetImageLabelInternal
On 10/7/19 11:49 PM, Cole Robinson wrote:
All the SetFileCon calls only differ by the label they pass in.
Rework the conditionals to track what label we need, and use a
single SetFileCon call
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/security/security_selinux.c | 31 ++++++++++---------------------
1 file changed, 10 insertions(+), 21 deletions(-)
Huh, I posted the same patch a while ago:
https://www.redhat.com/archives/libvir-list/2019-September/msg01240.html
Great minds think alike :-D
Michal
4:49 p.m.
New subject: [libvirt] [PATCH 26/30] security: selinux: Drop !parent handling in SetImageLabelInternal
The only caller always passes in a non-null parent
Signed-off-by: Cole Robinson <crobinso(a)redhat.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) {
--
2.23.0
11:05 a.m.
New subject: [libvirt] [PATCH 26/30] security: selinux: Drop !parent handling in SetImageLabelInternal
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(a)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(a)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) {
4:49 p.m.
New subject: [libvirt] [PATCH 27/30] security: selinux: Add is_toplevel to SetImageLabelInternal
This will simplify future patches and make the logic easier to follow
Signed-off-by: Cole Robinson <crobinso(a)redhat.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) {
--
2.23.0
11:07 a.m.
New subject: [libvirt] [PATCH 27/30] security: selinux: Add is_toplevel to SetImageLabelInternal
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(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)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) {
4:49 p.m.
New subject: [libvirt] [PATCH 28/30] security: selinux: Restore image label for externalDataStore
Rename the existing virSecuritySELinuxRestoreImageLabelInt
to virSecuritySELinuxRestoreImageLabelSingle, and extend the new
ImageLabelInt handle externalDataStore
Signed-off-by: Cole Robinson <crobinso(a)redhat.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,
--
2.23.0
11:09 a.m.
New subject: [libvirt] [PATCH 28/30] security: selinux: Restore image label for externalDataStore
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(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)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,
4:49 p.m.
New subject: [libvirt] [PATCH 29/30] security: selinux: break out SetImageLabelRelative
This will be used for recursing into externalDataStore
Signed-off-by: Cole Robinson <crobinso(a)redhat.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;
--
2.23.0
11:09 a.m.
New subject: [libvirt] [PATCH 29/30] security: selinux: break out SetImageLabelRelative
On 10/7/19 6:49 PM, Cole Robinson wrote:
This will be used for recursing into externalDataStore
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)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;
4:49 p.m.
New subject: [libvirt] [PATCH 30/30] security: selinux: Label externalDataStore
We mirror the labeling strategy that was used for its top image
Signed-off-by: Cole Robinson <crobinso(a)redhat.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;
}
--
2.23.0
11:11 a.m.
New subject: [libvirt] [PATCH 30/30] security: selinux: Label externalDataStore
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(a)redhat.com>
---
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)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;
}
12:52 a.m.
On Mon, Oct 7, 2019 at 11:49 PM Cole Robinson <crobinso(a)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-help...
src/libvirt_private.syms | 1 -
src/security/security_dac.c | 63 +++++--
src/security/security_selinux.c | 97 +++++++----
src/util/virstoragefile.c | 281 ++++++++++++++++++++------------
src/util/virstoragefile.h | 11 +-
5 files changed, 290 insertions(+), 163 deletions(-)
--
2.23.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
11:24 a.m.
On 10/8/19 1:52 AM, Christian Ehrhardt wrote:
On Mon, Oct 7, 2019 at 11:49 PM Cole Robinson
<crobinso(a)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-help...
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
11:22 a.m.
Hi,
ACKed basically everything, perhaps one or two patches I found something
worth talking about but nothing too gamebreaker. Logic-wise everything made
sense to me, but I believe someone else with a deeper understanding of the
storage backend in Libvirt might know better.
I am not sure how hard it is to test the changes you're making here, but it
would be good to have new stuff in virstoragetest.c (seems like the best
place)
to cover this new data_file support as well.
On a side note: from patches 20+ I got an impression that I was reviewing
the same patches over and over again. I think it's a good idea to have a
look
at the code repetition between the files in src/security dir, or at the
very least
between security_dac.c and security_selinux.c files.
Thanks,
DHB
On 10/7/19 6:49 PM, Cole Robinson 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
src/libvirt_private.syms | 1 -
src/security/security_dac.c | 63 +++++--
src/security/security_selinux.c | 97 +++++++----
src/util/virstoragefile.c | 281 ++++++++++++++++++++------------
src/util/virstoragefile.h | 11 +-
5 files changed, 290 insertions(+), 163 deletions(-)
10:25 p.m.
Hi Cole,
I merged crobinso/qcow2-data_file branch to 37b565c00. Reserved new
capabilities introduced by these to branches to resolve conflicts.
Then build and test as following:
# ./autogen.sh&& ./configure --without-libssh
--build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu
--program-prefix= --disable-dependency-tracking --prefix=/usr
--exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc
--datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64
--libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib
--mandir=/usr/share/man --infodir=/usr/share/info --with-qemu
--without-openvz --without-lxc --without-vbox --without-libxl --with-sasl
--with-polkit --with-libvirtd --without-phyp --with-esx --without-hyperv
--without-vmware --without-xenapi --without-vz --without-bhyve
--with-interface --with-network --with-storage-fs --with-storage-lvm
--with-storage-iscsi --with-storage-iscsi-direct --with-storage-scsi
--with-storage-disk --with-storage-mpath --with-storage-rbd
--without-storage-sheepdog --with-storage-gluster --without-storage-zfs
--without-storage-vstorage --with-numactl --with-numad --with-capng
--without-fuse --with-netcf --with-selinux
--with-selinux-mount=/sys/fs/selinux --without-apparmor --without-hal
--with-udev --with-yajl --with-sanlock --with-libpcap --with-macvtap
--with-audit --with-dtrace --with-driver-modules --with-firewalld
--with-firewalld-zone --without-wireshark-dissector --without-pm-utils
--with-nss-plugin '--with-packager=Unknown, 2019-08-19-12:13:01,
lab.rhel8.me' --with-packager-version=1.el8 --with-qemu-user=qemu
--with-qemu-group=qemu --with-tls-priority=@LIBVIRT,SYSTEM --enable-werror
--enable-expensive-tests --with-init-script=systemd --without-login-shell
&& make
Start libvirtd and virtlogd
# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')"
src/.libs/libvirtd
# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')"
./src/virtlogd
Then try to list all domains:
# virsh list --all
Libvirtd exits with segment fault:
[1] 30104 segmentation fault (core dumped) LD_PRELOAD="$(find src -name
'*.so.*'|tr '\n' ' ')" src/.libs/libvirtd
Version:
qemu-4.1
Backtrace:
(gdb) bt
#0 0x00007fbe57a0d1b9 in virDomainVirtioSerialAddrSetAddControllers
(def=<optimized out>, def=<optimized out>, addrs=<optimized out>) at
conf/domain_addr.c:1656
#1 virDomainVirtioSerialAddrSetCreateFromDomain (def=def@entry=0x7fbde81cc3f0)
at conf/domain_addr.c:1753
#2 0x00007fbe0179897e in qemuDomainAssignVirtioSerialAddresses
(def=0x7fbde81cc3f0) at qemu/qemu_domain_address.c:3174
#3 qemuDomainAssignAddresses (def=0x7fbde81cc3f0, qemuCaps=0x7fbde81d2210,
driver=0x7fbde8126850, obj=0x0, newDomain=<optimized out>) at
qemu/qemu_domain_address.c:3174
#4 0x00007fbe57a39e0d in virDomainDefPostParse (def=def@entry=0x7fbde81cc3f0,
caps=caps@entry=0x7fbde8154d20, parseFlags=parseFlags@entry=4610,
xmlopt=xmlopt@entry=0x7fbde83ce070,
parseOpaque=parseOpaque@entry=0x0) at conf/domain_conf.c:5858
#5 0x00007fbe57a525c5 in virDomainDefParseNode (xml=<optimized out>,
root=0x7fbde83c5ff0, caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070,
parseOpaque=0x0, flags=4610) at conf/domain_conf.c:21677
#6 0x00007fbe57a526c8 in virDomainDefParse (xmlStr=xmlStr@entry=0x0,
filename=<optimized out>, caps=caps@entry=0x7fbde8154d20,
xmlopt=xmlopt@entry=0x7fbde83ce070, parseOpaque=parseOpaque@entry=0x0,
flags=flags@entry=4610) at conf/domain_conf.c:21628
#7 0x00007fbe57a528f6 in virDomainDefParseFile (filename=<optimized out>,
caps=caps@entry=0x7fbde8154d20, xmlopt=xmlopt@entry=0x7fbde83ce070,
parseOpaque=parseOpaque@entry=0x0, flags=flags@entry=4610)
at conf/domain_conf.c:21653
#8 0x00007fbe57a5e16a in virDomainObjListLoadConfig (opaque=0x0,
notify=0x0, name=0x7fbde81d7ff3 "pc", autostartDir=0x7fbde8124070
"/etc/libvirt/qemu/autostart", configDir=0x7fbde8124050
"/etc/libvirt/qemu",
xmlopt=0x7fbde83ce070, caps=0x7fbde8154d20, doms=0x7fbde8126940) at
conf/virdomainobjlist.c:503
#9 virDomainObjListLoadAllConfigs (doms=0x7fbde8126940,
configDir=0x7fbde8124050 "/etc/libvirt/qemu", autostartDir=0x7fbde8124070
"/etc/libvirt/qemu/autostart", liveStatus=liveStatus@entry=false,
caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070, notify=0x0, opaque=0x0) at
conf/virdomainobjlist.c:625
#10 0x00007fbe017f57e2 in qemuStateInitialize (privileged=true,
callback=<optimized out>, opaque=<optimized out>) at
qemu/qemu_driver.c:1007
#11 0x00007fbe57b8033d in virStateInitialize (privileged=true,
mandatory=mandatory@entry=false, callback=callback@entry=0x55dfb702ecc0
<daemonInhibitCallback>, opaque=opaque@entry=0x55dfb8869d60)
at libvirt.c:666
#12 0x000055dfb702ed1d in daemonRunStateInit (opaque=0x55dfb8869d60) at
remote/remote_daemon.c:846
#13 0x00007fbe579f4be2 in virThreadHelper (data=<optimized out>) at
util/virthread.c:196
#14 0x00007fbe55a322de in start_thread (arg=<optimized out>) at
pthread_create.c:486
#15 0x00007fbe55763133 in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Could you please check this issue?
The full threads backtrace is in attachment
On Tue, Oct 8, 2019 at 5:49 AM Cole Robinson <crobinso(a)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
src/libvirt_private.syms | 1 -
src/security/security_dac.c | 63 +++++--
src/security/security_selinux.c | 97 +++++++----
src/util/virstoragefile.c | 281 ++++++++++++++++++++------------
src/util/virstoragefile.h | 11 +-
5 files changed, 290 insertions(+), 163 deletions(-)
--
2.23.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan(a)redhat.com
Phone: +861065339333
12:05 p.m.
On 10/10/19 11:25 PM, Han Han wrote:
Hi Cole,
I merged crobinso/qcow2-data_file branch to 37b565c00. Reserved new
capabilities introduced by these to branches to resolve conflicts.
Then build and test as following:
# ./autogen.sh&& ./configure --without-libssh
--build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu
--program-prefix= --disable-dependency-tracking --prefix=/usr
--exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin
--sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include
--libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var
--sharedstatedir=/var/lib --mandir=/usr/share/man
--infodir=/usr/share/info --with-qemu --without-openvz --without-lxc
--without-vbox --without-libxl --with-sasl --with-polkit --with-libvirtd
--without-phyp --with-esx --without-hyperv --without-vmware
--without-xenapi --without-vz --without-bhyve --with-interface
--with-network --with-storage-fs --with-storage-lvm --with-storage-iscsi
--with-storage-iscsi-direct --with-storage-scsi --with-storage-disk
--with-storage-mpath --with-storage-rbd --without-storage-sheepdog
--with-storage-gluster --without-storage-zfs --without-storage-vstorage
--with-numactl --with-numad --with-capng --without-fuse --with-netcf
--with-selinux --with-selinux-mount=/sys/fs/selinux --without-apparmor
--without-hal --with-udev --with-yajl --with-sanlock --with-libpcap
--with-macvtap --with-audit --with-dtrace --with-driver-modules
--with-firewalld --with-firewalld-zone --without-wireshark-dissector
--without-pm-utils --with-nss-plugin '--with-packager=Unknown,
2019-08-19-12:13:01, lab.rhel8.me <http://lab.rhel8.me>'
--with-packager-version=1.el8 --with-qemu-user=qemu
--with-qemu-group=qemu --with-tls-priority=@LIBVIRT,SYSTEM
--enable-werror --enable-expensive-tests --with-init-script=systemd
--without-login-shell && make
Start libvirtd and virtlogd
# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')"
src/.libs/libvirtd
# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')"
./src/virtlogd
Then try to list all domains:
# virsh list --all
Libvirtd exits with segment fault:
[1] 30104 segmentation fault (core dumped) LD_PRELOAD="$(find src
-name '*.so.*'|tr '\n' ' ')" src/.libs/libvirtd
Version:
qemu-4.1
Backtrace:
(gdb) bt
#0 0x00007fbe57a0d1b9 in virDomainVirtioSerialAddrSetAddControllers
(def=<optimized out>, def=<optimized out>, addrs=<optimized out>) at
conf/domain_addr.c:1656
#1 virDomainVirtioSerialAddrSetCreateFromDomain
(def=def@entry=0x7fbde81cc3f0) at conf/domain_addr.c:1753
#2 0x00007fbe0179897e in qemuDomainAssignVirtioSerialAddresses
(def=0x7fbde81cc3f0) at qemu/qemu_domain_address.c:3174
#3 qemuDomainAssignAddresses (def=0x7fbde81cc3f0,
qemuCaps=0x7fbde81d2210, driver=0x7fbde8126850, obj=0x0,
newDomain=<optimized out>) at qemu/qemu_domain_address.c:3174
#4 0x00007fbe57a39e0d in virDomainDefPostParse
(def=def@entry=0x7fbde81cc3f0, caps=caps@entry=0x7fbde8154d20,
parseFlags=parseFlags@entry=4610, xmlopt=xmlopt@entry=0x7fbde83ce070,
parseOpaque=parseOpaque@entry=0x0) at conf/domain_conf.c:5858
#5 0x00007fbe57a525c5 in virDomainDefParseNode (xml=<optimized out>,
root=0x7fbde83c5ff0, caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070,
parseOpaque=0x0, flags=4610) at conf/domain_conf.c:21677
#6 0x00007fbe57a526c8 in virDomainDefParse (xmlStr=xmlStr@entry=0x0,
filename=<optimized out>, caps=caps@entry=0x7fbde8154d20,
xmlopt=xmlopt@entry=0x7fbde83ce070, parseOpaque=parseOpaque@entry=0x0,
flags=flags@entry=4610) at conf/domain_conf.c:21628
#7 0x00007fbe57a528f6 in virDomainDefParseFile (filename=<optimized
out>, caps=caps@entry=0x7fbde8154d20,
xmlopt=xmlopt@entry=0x7fbde83ce070, parseOpaque=parseOpaque@entry=0x0,
flags=flags@entry=4610)
at conf/domain_conf.c:21653
#8 0x00007fbe57a5e16a in virDomainObjListLoadConfig (opaque=0x0,
notify=0x0, name=0x7fbde81d7ff3 "pc", autostartDir=0x7fbde8124070
"/etc/libvirt/qemu/autostart", configDir=0x7fbde8124050
"/etc/libvirt/qemu",
xmlopt=0x7fbde83ce070, caps=0x7fbde8154d20, doms=0x7fbde8126940) at
conf/virdomainobjlist.c:503
#9 virDomainObjListLoadAllConfigs (doms=0x7fbde8126940,
configDir=0x7fbde8124050 "/etc/libvirt/qemu",
autostartDir=0x7fbde8124070 "/etc/libvirt/qemu/autostart",
liveStatus=liveStatus@entry=false,
caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070, notify=0x0, opaque=0x0)
at conf/virdomainobjlist.c:625
#10 0x00007fbe017f57e2 in qemuStateInitialize (privileged=true,
callback=<optimized out>, opaque=<optimized out>) at
qemu/qemu_driver.c:1007
#11 0x00007fbe57b8033d in virStateInitialize (privileged=true,
mandatory=mandatory@entry=false, callback=callback@entry=0x55dfb702ecc0
<daemonInhibitCallback>, opaque=opaque@entry=0x55dfb8869d60)
at libvirt.c:666
#12 0x000055dfb702ed1d in daemonRunStateInit (opaque=0x55dfb8869d60) at
remote/remote_daemon.c:846
#13 0x00007fbe579f4be2 in virThreadHelper (data=<optimized out>) at
util/virthread.c:196
#14 0x00007fbe55a322de in start_thread (arg=<optimized out>) at
pthread_create.c:486
#15 0x00007fbe55763133 in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Could you please check this issue?
The full threads backtrace is in attachment
Thanks for checking. I'm struggling to see how that backtrace could be
related to this series, or even determine what the issue is. Can you
confirm that master branch doesn't have any issue? If it doesn't, can
you bisect the series to figure out where the offending patch is?
Thanks,
Cole
10:07 a.m.
On Sat, Oct 12, 2019 at 1:05 AM Cole Robinson <crobinso(a)redhat.com> wrote:
On 10/10/19 11:25 PM, Han Han wrote:
> Hi Cole,
> I merged crobinso/qcow2-data_file branch to 37b565c00. Reserved new
> capabilities introduced by these to branches to resolve conflicts.
> Then build and test as following:
> # ./autogen.sh&& ./configure --without-libssh
> --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu
> --program-prefix= --disable-dependency-tracking --prefix=/usr
> --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin
> --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include
> --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var
> --sharedstatedir=/var/lib --mandir=/usr/share/man
> --infodir=/usr/share/info --with-qemu --without-openvz --without-lxc
> --without-vbox --without-libxl --with-sasl --with-polkit --with-libvirtd
> --without-phyp --with-esx --without-hyperv --without-vmware
> --without-xenapi --without-vz --without-bhyve --with-interface
> --with-network --with-storage-fs --with-storage-lvm --with-storage-iscsi
> --with-storage-iscsi-direct --with-storage-scsi --with-storage-disk
> --with-storage-mpath --with-storage-rbd --without-storage-sheepdog
> --with-storage-gluster --without-storage-zfs --without-storage-vstorage
> --with-numactl --with-numad --with-capng --without-fuse --with-netcf
> --with-selinux --with-selinux-mount=/sys/fs/selinux --without-apparmor
> --without-hal --with-udev --with-yajl --with-sanlock --with-libpcap
> --with-macvtap --with-audit --with-dtrace --with-driver-modules
> --with-firewalld --with-firewalld-zone --without-wireshark-dissector
> --without-pm-utils --with-nss-plugin '--with-packager=Unknown,
> 2019-08-19-12:13:01, lab.rhel8.me <http://lab.rhel8.me>'
> --with-packager-version=1.el8 --with-qemu-user=qemu
> --with-qemu-group=qemu --with-tls-priority=@LIBVIRT,SYSTEM
> --enable-werror --enable-expensive-tests --with-init-script=systemd
> --without-login-shell && make
>
> Start libvirtd and virtlogd
> # LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' '
')" src/.libs/libvirtd
> # LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' '
')" ./src/virtlogd
>
> Then try to list all domains:
> # virsh list --all
>
> Libvirtd exits with segment fault:
> [1] 30104 segmentation fault (core dumped) LD_PRELOAD="$(find src
> -name '*.so.*'|tr '\n' ' ')" src/.libs/libvirtd
>
> Version:
> qemu-4.1
>
> Backtrace:
> (gdb) bt
> #0 0x00007fbe57a0d1b9 in virDomainVirtioSerialAddrSetAddControllers
> (def=<optimized out>, def=<optimized out>, addrs=<optimized out>)
at
> conf/domain_addr.c:1656
> #1 virDomainVirtioSerialAddrSetCreateFromDomain
> (def=def@entry=0x7fbde81cc3f0) at conf/domain_addr.c:1753
> #2 0x00007fbe0179897e in qemuDomainAssignVirtioSerialAddresses
> (def=0x7fbde81cc3f0) at qemu/qemu_domain_address.c:3174
> #3 qemuDomainAssignAddresses (def=0x7fbde81cc3f0,
> qemuCaps=0x7fbde81d2210, driver=0x7fbde8126850, obj=0x0,
> newDomain=<optimized out>) at qemu/qemu_domain_address.c:3174
> #4 0x00007fbe57a39e0d in virDomainDefPostParse
> (def=def@entry=0x7fbde81cc3f0, caps=caps@entry=0x7fbde8154d20,
> parseFlags=parseFlags@entry=4610, xmlopt=xmlopt@entry=0x7fbde83ce070,
> parseOpaque=parseOpaque@entry=0x0) at conf/domain_conf.c:5858
> #5 0x00007fbe57a525c5 in virDomainDefParseNode (xml=<optimized out>,
> root=0x7fbde83c5ff0, caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070,
> parseOpaque=0x0, flags=4610) at conf/domain_conf.c:21677
> #6 0x00007fbe57a526c8 in virDomainDefParse (xmlStr=xmlStr@entry=0x0,
> filename=<optimized out>, caps=caps@entry=0x7fbde8154d20,
> xmlopt=xmlopt@entry=0x7fbde83ce070, parseOpaque=parseOpaque@entry=0x0,
> flags=flags@entry=4610) at conf/domain_conf.c:21628
> #7 0x00007fbe57a528f6 in virDomainDefParseFile (filename=<optimized
> out>, caps=caps@entry=0x7fbde8154d20,
> xmlopt=xmlopt@entry=0x7fbde83ce070, parseOpaque=parseOpaque@entry=0x0,
> flags=flags@entry=4610)
> at conf/domain_conf.c:21653
> #8 0x00007fbe57a5e16a in virDomainObjListLoadConfig (opaque=0x0,
> notify=0x0, name=0x7fbde81d7ff3 "pc", autostartDir=0x7fbde8124070
> "/etc/libvirt/qemu/autostart", configDir=0x7fbde8124050
> "/etc/libvirt/qemu",
> xmlopt=0x7fbde83ce070, caps=0x7fbde8154d20, doms=0x7fbde8126940) at
> conf/virdomainobjlist.c:503
> #9 virDomainObjListLoadAllConfigs (doms=0x7fbde8126940,
> configDir=0x7fbde8124050 "/etc/libvirt/qemu",
> autostartDir=0x7fbde8124070 "/etc/libvirt/qemu/autostart",
> liveStatus=liveStatus@entry=false,
> caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070, notify=0x0, opaque=0x0)
> at conf/virdomainobjlist.c:625
> #10 0x00007fbe017f57e2 in qemuStateInitialize (privileged=true,
> callback=<optimized out>, opaque=<optimized out>) at
> qemu/qemu_driver.c:1007
> #11 0x00007fbe57b8033d in virStateInitialize (privileged=true,
> mandatory=mandatory@entry=false, callback=callback@entry=0x55dfb702ecc0
> <daemonInhibitCallback>, opaque=opaque@entry=0x55dfb8869d60)
> at libvirt.c:666
> #12 0x000055dfb702ed1d in daemonRunStateInit (opaque=0x55dfb8869d60) at
> remote/remote_daemon.c:846
> #13 0x00007fbe579f4be2 in virThreadHelper (data=<optimized out>) at
> util/virthread.c:196
> #14 0x00007fbe55a322de in start_thread (arg=<optimized out>) at
> pthread_create.c:486
> #15 0x00007fbe55763133 in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
>
> Could you please check this issue?
> The full threads backtrace is in attachment
>
Hello, the git bisect shows that is the first bad commit:
192229f3a76ccc1b98a2c9e24f1feb0465b87a0b is the first bad commit
commit 192229f3a76ccc1b98a2c9e24f1feb0465b87a0b
Author: Cole Robinson <crobinso(a)redhat.com>
Date: Fri Oct 4 19:57:55 2019 -0400
storagefile: Push extension_end calc to qcow2GetBackingStoreFormat
This is a step towards making this qcow2GetBackingStoreFormat into
a generic qcow2 extensions parser
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
Steps:
1. Merge crobinso/qcow2-data_file branch to 37b565c00.
2. Copy .gdbinit to libvirt source dir. Change the arguments values of
check-segv.sh
3. Set v5.8.0 as the start of bisect. Then start bisect.
# git bisect start HEAD v5.8.0
# git bisect run /tmp/check-segv.sh
Thanks for checking. I'm struggling to see how that backtrace
could be
related to this series, or even determine what the issue is. Can you
confirm that master branch doesn't have any issue? If it doesn't, can
you bisect the series to figure out where the offending patch is?
Thanks,
Cole
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan(a)redhat.com
Phone: +861065339333
2:56 p.m.
On 10/12/19 11:07 AM, Han Han wrote:
On Sat, Oct 12, 2019 at 1:05 AM Cole Robinson <crobinso(a)redhat.com
<mailto:crobinso@redhat.com>> wrote:
On 10/10/19 11:25 PM, Han Han wrote:
> Hi Cole,
> I merged crobinso/qcow2-data_file branch to 37b565c00. Reserved new
> capabilities introduced by these to branches to resolve conflicts.
> Then build and test as following:
> # ./autogen.sh&& ./configure --without-libssh
> --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu
> --program-prefix= --disable-dependency-tracking --prefix=/usr
> --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin
> --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include
> --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var
> --sharedstatedir=/var/lib --mandir=/usr/share/man
> --infodir=/usr/share/info --with-qemu --without-openvz --without-lxc
> --without-vbox --without-libxl --with-sasl --with-polkit
--with-libvirtd
> --without-phyp --with-esx --without-hyperv --without-vmware
> --without-xenapi --without-vz --without-bhyve --with-interface
> --with-network --with-storage-fs --with-storage-lvm
--with-storage-iscsi
> --with-storage-iscsi-direct --with-storage-scsi --with-storage-disk
> --with-storage-mpath --with-storage-rbd --without-storage-sheepdog
> --with-storage-gluster --without-storage-zfs
--without-storage-vstorage
> --with-numactl --with-numad --with-capng --without-fuse --with-netcf
> --with-selinux --with-selinux-mount=/sys/fs/selinux
--without-apparmor
> --without-hal --with-udev --with-yajl --with-sanlock --with-libpcap
> --with-macvtap --with-audit --with-dtrace --with-driver-modules
> --with-firewalld --with-firewalld-zone --without-wireshark-dissector
> --without-pm-utils --with-nss-plugin '--with-packager=Unknown,
> 2019-08-19-12:13:01, lab.rhel8.me <http://lab.rhel8.me>
<http://lab.rhel8.me>'
> --with-packager-version=1.el8 --with-qemu-user=qemu
> --with-qemu-group=qemu --with-tls-priority=@LIBVIRT,SYSTEM
> --enable-werror --enable-expensive-tests --with-init-script=systemd
> --without-login-shell && make
>
> Start libvirtd and virtlogd
> # LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' '
')"
src/.libs/libvirtd
> # LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' '
')" ./src/virtlogd
>
> Then try to list all domains:
> # virsh list --all
>
> Libvirtd exits with segment fault:
> [1] 30104 segmentation fault (core dumped) LD_PRELOAD="$(find src
> -name '*.so.*'|tr '\n' ' ')" src/.libs/libvirtd
>
> Version:
> qemu-4.1
>
> Backtrace:
> (gdb) bt
> #0 0x00007fbe57a0d1b9 in virDomainVirtioSerialAddrSetAddControllers
> (def=<optimized out>, def=<optimized out>, addrs=<optimized
out>) at
> conf/domain_addr.c:1656
> #1 virDomainVirtioSerialAddrSetCreateFromDomain
> (def=def@entry=0x7fbde81cc3f0) at conf/domain_addr.c:1753
> #2 0x00007fbe0179897e in qemuDomainAssignVirtioSerialAddresses
> (def=0x7fbde81cc3f0) at qemu/qemu_domain_address.c:3174
> #3 qemuDomainAssignAddresses (def=0x7fbde81cc3f0,
> qemuCaps=0x7fbde81d2210, driver=0x7fbde8126850, obj=0x0,
> newDomain=<optimized out>) at qemu/qemu_domain_address.c:3174
> #4 0x00007fbe57a39e0d in virDomainDefPostParse
> (def=def@entry=0x7fbde81cc3f0, caps=caps@entry=0x7fbde8154d20,
> parseFlags=parseFlags@entry=4610, xmlopt=xmlopt@entry=0x7fbde83ce070,
> parseOpaque=parseOpaque@entry=0x0) at conf/domain_conf.c:5858
> #5 0x00007fbe57a525c5 in virDomainDefParseNode (xml=<optimized out>,
> root=0x7fbde83c5ff0, caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070,
> parseOpaque=0x0, flags=4610) at conf/domain_conf.c:21677
> #6 0x00007fbe57a526c8 in virDomainDefParse (xmlStr=xmlStr@entry=0x0,
> filename=<optimized out>, caps=caps@entry=0x7fbde8154d20,
> xmlopt=xmlopt@entry=0x7fbde83ce070, parseOpaque=parseOpaque@entry=0x0,
> flags=flags@entry=4610) at conf/domain_conf.c:21628
> #7 0x00007fbe57a528f6 in virDomainDefParseFile (filename=<optimized
> out>, caps=caps@entry=0x7fbde8154d20,
> xmlopt=xmlopt@entry=0x7fbde83ce070,
parseOpaque=parseOpaque@entry=0x0,
> flags=flags@entry=4610)
> at conf/domain_conf.c:21653
> #8 0x00007fbe57a5e16a in virDomainObjListLoadConfig (opaque=0x0,
> notify=0x0, name=0x7fbde81d7ff3 "pc", autostartDir=0x7fbde8124070
> "/etc/libvirt/qemu/autostart", configDir=0x7fbde8124050
> "/etc/libvirt/qemu",
> xmlopt=0x7fbde83ce070, caps=0x7fbde8154d20,
doms=0x7fbde8126940) at
> conf/virdomainobjlist.c:503
> #9 virDomainObjListLoadAllConfigs (doms=0x7fbde8126940,
> configDir=0x7fbde8124050 "/etc/libvirt/qemu",
> autostartDir=0x7fbde8124070 "/etc/libvirt/qemu/autostart",
> liveStatus=liveStatus@entry=false,
> caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070, notify=0x0,
opaque=0x0)
> at conf/virdomainobjlist.c:625
> #10 0x00007fbe017f57e2 in qemuStateInitialize (privileged=true,
> callback=<optimized out>, opaque=<optimized out>) at
> qemu/qemu_driver.c:1007
> #11 0x00007fbe57b8033d in virStateInitialize (privileged=true,
> mandatory=mandatory@entry=false,
callback=callback@entry=0x55dfb702ecc0
> <daemonInhibitCallback>, opaque=opaque@entry=0x55dfb8869d60)
> at libvirt.c:666
> #12 0x000055dfb702ed1d in daemonRunStateInit
(opaque=0x55dfb8869d60) at
> remote/remote_daemon.c:846
> #13 0x00007fbe579f4be2 in virThreadHelper (data=<optimized out>) at
> util/virthread.c:196
> #14 0x00007fbe55a322de in start_thread (arg=<optimized out>) at
> pthread_create.c:486
> #15 0x00007fbe55763133 in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
>
> Could you please check this issue?
> The full threads backtrace is in attachment
>
Hello, the git bisect shows that is the first bad commit:
192229f3a76ccc1b98a2c9e24f1feb0465b87a0b is the first bad commit
commit 192229f3a76ccc1b98a2c9e24f1feb0465b87a0b
Author: Cole Robinson <crobinso(a)redhat.com <mailto:crobinso@redhat.com>>
Date: Fri Oct 4 19:57:55 2019 -0400
storagefile: Push extension_end calc to qcow2GetBackingStoreFormat
This is a step towards making this qcow2GetBackingStoreFormat into
a generic qcow2 extensions parser
Signed-off-by: Cole Robinson <crobinso(a)redhat.com
<mailto:crobinso@redhat.com>>
Steps:
1. Merge crobinso/qcow2-data_file branch to 37b565c00.
2. Copy .gdbinit to libvirt source dir. Change the arguments values of
check-segv.sh
3. Set v5.8.0 as the start of bisect. Then start bisect.
# git bisect start HEAD v5.8.0
# git bisect run /tmp/check-segv.sh
I'm still quite confused. Maybe something I'm missing in one of these
commits is causing memory corruption that is manifesting elsewhere?
Can you provide full LIBVIRT_DEBUG=1 output when starting libvirtd? You
can use git master now because the patches have been pushed. I suggest
hosting the output somewhere rather than attaching it here, because it
will probably be large
Also, if you can pinpoint what VM XML that is being parsed when this
crashes, and post that too, it might help.
Thanks,
Cole
11:29 p.m.
I find the issue cannot reproduced when `make clean` before build the
source.
It is not proper to build with an unclean source dir, right?
On Tue, Oct 15, 2019 at 3:55 AM Cole Robinson <crobinso(a)redhat.com> wrote:
On 10/12/19 11:07 AM, Han Han wrote:
>
>
> On Sat, Oct 12, 2019 at 1:05 AM Cole Robinson <crobinso(a)redhat.com
> <mailto:crobinso@redhat.com>> wrote:
>
> On 10/10/19 11:25 PM, Han Han wrote:
> > Hi Cole,
> > I merged crobinso/qcow2-data_file branch to 37b565c00. Reserved new
> > capabilities introduced by these to branches to resolve conflicts.
> > Then build and test as following:
> > # ./autogen.sh&& ./configure --without-libssh
> > --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu
> > --program-prefix= --disable-dependency-tracking --prefix=/usr
> > --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin
> > --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include
> > --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var
> > --sharedstatedir=/var/lib --mandir=/usr/share/man
> > --infodir=/usr/share/info --with-qemu --without-openvz
--without-lxc
> > --without-vbox --without-libxl --with-sasl --with-polkit
> --with-libvirtd
> > --without-phyp --with-esx --without-hyperv --without-vmware
> > --without-xenapi --without-vz --without-bhyve --with-interface
> > --with-network --with-storage-fs --with-storage-lvm
> --with-storage-iscsi
> > --with-storage-iscsi-direct --with-storage-scsi --with-storage-disk
> > --with-storage-mpath --with-storage-rbd --without-storage-sheepdog
> > --with-storage-gluster --without-storage-zfs
> --without-storage-vstorage
> > --with-numactl --with-numad --with-capng --without-fuse
--with-netcf
> > --with-selinux --with-selinux-mount=/sys/fs/selinux
> --without-apparmor
> > --without-hal --with-udev --with-yajl --with-sanlock --with-libpcap
> > --with-macvtap --with-audit --with-dtrace --with-driver-modules
> > --with-firewalld --with-firewalld-zone
--without-wireshark-dissector
> > --without-pm-utils --with-nss-plugin '--with-packager=Unknown,
> > 2019-08-19-12:13:01, lab.rhel8.me <http://lab.rhel8.me>
> <http://lab.rhel8.me>'
> > --with-packager-version=1.el8 --with-qemu-user=qemu
> > --with-qemu-group=qemu --with-tls-priority=@LIBVIRT,SYSTEM
> > --enable-werror --enable-expensive-tests --with-init-script=systemd
> > --without-login-shell && make
> >
> > Start libvirtd and virtlogd
> > # LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' '
')"
> src/.libs/libvirtd
> > # LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' '
')"
./src/virtlogd
> >
> > Then try to list all domains:
> > # virsh list --all
> >
> > Libvirtd exits with segment fault:
> > [1] 30104 segmentation fault (core dumped) LD_PRELOAD="$(find
src
> > -name '*.so.*'|tr '\n' ' ')"
src/.libs/libvirtd
> >
> > Version:
> > qemu-4.1
> >
> > Backtrace:
> > (gdb) bt
> > #0 0x00007fbe57a0d1b9 in
virDomainVirtioSerialAddrSetAddControllers
> > (def=<optimized out>, def=<optimized out>, addrs=<optimized
out>)
at
> > conf/domain_addr.c:1656
> > #1 virDomainVirtioSerialAddrSetCreateFromDomain
> > (def=def@entry=0x7fbde81cc3f0) at conf/domain_addr.c:1753
> > #2 0x00007fbe0179897e in qemuDomainAssignVirtioSerialAddresses
> > (def=0x7fbde81cc3f0) at qemu/qemu_domain_address.c:3174
> > #3 qemuDomainAssignAddresses (def=0x7fbde81cc3f0,
> > qemuCaps=0x7fbde81d2210, driver=0x7fbde8126850, obj=0x0,
> > newDomain=<optimized out>) at qemu/qemu_domain_address.c:3174
> > #4 0x00007fbe57a39e0d in virDomainDefPostParse
> > (def=def@entry=0x7fbde81cc3f0, caps=caps@entry=0x7fbde8154d20,
> > parseFlags=parseFlags@entry=4610, xmlopt=xmlopt@entry
=0x7fbde83ce070,
> > parseOpaque=parseOpaque@entry=0x0) at conf/domain_conf.c:5858
> > #5 0x00007fbe57a525c5 in virDomainDefParseNode (xml=<optimized
out>,
> > root=0x7fbde83c5ff0, caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070,
> > parseOpaque=0x0, flags=4610) at conf/domain_conf.c:21677
> > #6 0x00007fbe57a526c8 in virDomainDefParse (xmlStr=xmlStr@entry
=0x0,
> > filename=<optimized out>, caps=caps@entry=0x7fbde8154d20,
> > xmlopt=xmlopt@entry=0x7fbde83ce070, parseOpaque=parseOpaque@entry
=0x0,
> > flags=flags@entry=4610) at conf/domain_conf.c:21628
> > #7 0x00007fbe57a528f6 in virDomainDefParseFile
(filename=<optimized
> > out>, caps=caps@entry=0x7fbde8154d20,
> > xmlopt=xmlopt@entry=0x7fbde83ce070,
> parseOpaque=parseOpaque@entry=0x0,
> > flags=flags@entry=4610)
> > at conf/domain_conf.c:21653
> > #8 0x00007fbe57a5e16a in virDomainObjListLoadConfig (opaque=0x0,
> > notify=0x0, name=0x7fbde81d7ff3 "pc",
autostartDir=0x7fbde8124070
> > "/etc/libvirt/qemu/autostart", configDir=0x7fbde8124050
> > "/etc/libvirt/qemu",
> > xmlopt=0x7fbde83ce070, caps=0x7fbde8154d20,
> doms=0x7fbde8126940) at
> > conf/virdomainobjlist.c:503
> > #9 virDomainObjListLoadAllConfigs (doms=0x7fbde8126940,
> > configDir=0x7fbde8124050 "/etc/libvirt/qemu",
> > autostartDir=0x7fbde8124070 "/etc/libvirt/qemu/autostart",
> > liveStatus=liveStatus@entry=false,
> > caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070, notify=0x0,
> opaque=0x0)
> > at conf/virdomainobjlist.c:625
> > #10 0x00007fbe017f57e2 in qemuStateInitialize (privileged=true,
> > callback=<optimized out>, opaque=<optimized out>) at
> > qemu/qemu_driver.c:1007
> > #11 0x00007fbe57b8033d in virStateInitialize (privileged=true,
> > mandatory=mandatory@entry=false,
> callback=callback@entry=0x55dfb702ecc0
> > <daemonInhibitCallback>, opaque=opaque@entry=0x55dfb8869d60)
> > at libvirt.c:666
> > #12 0x000055dfb702ed1d in daemonRunStateInit
> (opaque=0x55dfb8869d60) at
> > remote/remote_daemon.c:846
> > #13 0x00007fbe579f4be2 in virThreadHelper (data=<optimized out>) at
> > util/virthread.c:196
> > #14 0x00007fbe55a322de in start_thread (arg=<optimized out>) at
> > pthread_create.c:486
> > #15 0x00007fbe55763133 in clone () at
> > ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
> >
> > Could you please check this issue?
> > The full threads backtrace is in attachment
> >
>
> Hello, the git bisect shows that is the first bad commit:
> 192229f3a76ccc1b98a2c9e24f1feb0465b87a0b is the first bad commit
> commit 192229f3a76ccc1b98a2c9e24f1feb0465b87a0b
> Author: Cole Robinson <crobinso(a)redhat.com
<mailto:crobinso@redhat.com>>
> Date: Fri Oct 4 19:57:55 2019 -0400
>
> storagefile: Push extension_end calc to qcow2GetBackingStoreFormat
>
> This is a step towards making this qcow2GetBackingStoreFormat into
> a generic qcow2 extensions parser
>
> Signed-off-by: Cole Robinson <crobinso(a)redhat.com
> <mailto:crobinso@redhat.com>>
>
>
> Steps:
> 1. Merge crobinso/qcow2-data_file branch to 37b565c00.
> 2. Copy .gdbinit to libvirt source dir. Change the arguments values of
> check-segv.sh
> 3. Set v5.8.0 as the start of bisect. Then start bisect.
> # git bisect start HEAD v5.8.0
> # git bisect run /tmp/check-segv.sh
>
I'm still quite confused. Maybe something I'm missing in one of these
commits is causing memory corruption that is manifesting elsewhere?
Can you provide full LIBVIRT_DEBUG=1 output when starting libvirtd? You
can use git master now because the patches have been pushed. I suggest
hosting the output somewhere rather than attaching it here, because it
will probably be large
Also, if you can pinpoint what VM XML that is being parsed when this
crashes, and post that too, it might help.
Thanks,
Cole
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan(a)redhat.com
Phone: +861065339333
9:21 a.m.
On 10/15/19 12:29 AM, Han Han wrote:
I find the issue cannot reproduced when `make clean` before build
the
source.
It is not proper to build with an unclean source dir, right?
I don't use 'make clean' in libvirt.git but in other projects I have hit
issues that required 'make clean', like in qemu if trying to run old
commits, often the build system gets confused. I dip in and out of
libvirt development though so maybe regular devs have hit similar issues
- Cole
8:04 a.m.
On 10/7/19 11:49 PM, Cole Robinson 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
src/libvirt_private.syms | 1 -
src/security/security_dac.c | 63 +++++--
src/security/security_selinux.c | 97 +++++++----
src/util/virstoragefile.c | 281 ++++++++++++++++++++------------
src/util/virstoragefile.h | 11 +-
5 files changed, 290 insertions(+), 163 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
8:45 a.m.
On Mon, Oct 07, 2019 at 05:49:14PM -0400, Cole Robinson 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
This also belongs in the persistent 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)
<metadataStore> maybe?
* 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.
Historically, not being explicit on the command line and letting QEMU
do the right thing has bitten us, so yes, we have to do it 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.
If the point of the thin qcow2 layer is to contain the dirty bitmaps for
incremental backup then running this then you might as well use a
different metadata_file? Otherwise the metadata won't match the actual
data.
OTOH, I can imagine throwing away the metadata file and starting over.
* 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.
This will be fixed by introducing an XML element for it.
Jano
Most of this is cleanups and refactorings to simplify the actual
functional changes.
12:31 p.m.
On 10/11/19 9:45 AM, Ján Tomko wrote:
On Mon, Oct 07, 2019 at 05:49:14PM -0400, Cole Robinson 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
This also belongs in the persistent XML.
Agreed
> 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)
>
<metadataStore> maybe?
The way this code is structured, we have
src->path = FOO.qcow2
src->externalDataStore-> FOO.raw
FOO.raw contains the disk/OS contents, FOO.qcow2 just the qcow2
metadata. If we reflect that layout in the XML, we have
<disk>
<source file='FOO.qcow'>
<externalDataStore>
<source file='FOO.raw'/>
</externalDataStore>
</source>
</disk>
If we called it metadataStore it sounds like the layout is inverted.
>> * 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.
Historically, not being explicit on the command line and letting QEMU
do the right thing has bitten us, so yes, we have to do it 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.
>
If the point of the thin qcow2 layer is to contain the dirty bitmaps for
incremental backup then running this then you might as well use a
different metadata_file? Otherwise the metadata won't match the actual
data.
I'm not sure I follow this part, but maybe that's due to data_file
naming mixup
OTOH, I can imagine throwing away the metadata file and starting
over.
Yes this is one of the main drivers I think. That the qcow2 layer gives
qcow2 native features like dirty bitmaps, but if it ever comes to it,
the data is still in raw format which simplifies processing the image
with other tools. Plus raw is less of a boogieman than qcow2 for some
people, so I think there's some marketing opportunity behind it to say
'see your data is still there in FOO.raw'.
There's probably cases where the user would want to ditch the top level
layer and use that data raw layer directly, but similar to writing to a
backing image, it invalidates the top layer, and there's no rebase
operation for data_file AFAICT. But the persistent XML will allow
configuring that if someone wanted it
> * 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.
This will be fixed by introducing an XML element for it.
It's part of the fix I think. We will still need to change qemu_block.c
logic to accomodate this in some way. Right now, whether we probe the
qcow2 file metadata is only dependent on <backingStore> in the
persistent XML or not. But now the probing provides info on both
backingStore and externalDataStore, so tying probing only to prescence
of backingStore XML isn't sufficient.
I'm thinking extend the storage_file.c entry points to have an option
like 'skipBackingStore' and 'skipExternalDataStore' or similar, so we
only probe what we want, and probing is skipped entirely only if both
backingStore and externalDataStore are in the XML. That's just an idea,
I'll look into it more next week and if there's no clear answer I'll
start a separate thread
Thanks,
Cole
2:56 a.m.
Hello Cole, one issue is found:
The qcow2 data file XTTRs is not cleaned on external snapshot when
-blockdev is not enabled
Versions:
libvirt v5.8.0-134-g9d03e9adf1
qemu-kvm-4.1.0-13.module+el8.1.0+4313+ef76ec61.x86_64
Steps:
1. Convert a OS image to qcow2&qcow2 data file:
# qemu-img convert -O qcow2 -o
data_file=/var/lib/libvirt/images/pc-data.raw,data_file_raw=on
/var/lib/libvirt/images/pc.qcow2 /var/lib/libvirt/images/pc-data.qcow2
2. Build and start libvirt source, start libvirt daemon:
# make clean && CC=/usr/lib64/ccache/cc ./autogen.sh&&./configure
--without-libssh --build=x86_64-redhat-linux-gnu
--host=x86_64-redhat-linux-gnu --program-prefix=
--disable-dependency-tracking --prefix=/usr --exec-prefix=/usr
--bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc
--datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64
--libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib
--mandir=/usr/share/man --infodir=/usr/share/info --with-qemu
--without-openvz --without-lxc --without-vbox --without-libxl --with-sasl
--with-polkit --with-libvirtd --without-phyp --with-esx --without-hyperv
--without-vmware --without-xenapi --without-vz --without-bhyve
--with-interface --with-network --with-storage-fs --with-storage-lvm
--with-storage-iscsi --with-storage-iscsi-direct --with-storage-scsi
--with-storage-disk --with-storage-mpath --with-storage-rbd
--without-storage-sheepdog --with-storage-gluster --without-storage-zfs
--without-storage-vstorage --with-numactl --with-numad --with-capng
--without-fuse --with-netcf --with-selinux
--with-selinux-mount=/sys/fs/selinux --without-apparmor --without-hal
--with-udev --with-yajl --with-sanlock --with-libpcap --with-macvtap
--with-audit --with-dtrace --with-driver-modules --with-firewalld
--with-firewalld-zone --without-wireshark-dissector --without-pm-utils
--with-nss-plugin '--with-packager=Unknown, 2019-08-19-12:13:01,
lab.rhel8.me' --with-packager-version=1.el8 --with-qemu-user=qemu
--with-qemu-group=qemu --with-tls-priority=@LIBVIRT,SYSTEM --enable-werror
--enable-expensive-tests --with-init-script=systemd --without-login-shell
&& make -j8
# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')"
src/.libs/virtlogd
# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')"
LIBVIRT_DEBUG=3
LIBVIRT_LOG_FILTERS="1:util 1:qemu 1:security"
LIBVIRT_LOG_OUTPUTS="1:file:/tmp/libvirt_daemon.log" src/.libs/libvirtd
3. Define and start an VM with the qcow2&qcow2 data file. Note that the
-blockdev is not enabled
# virsh define pc-data.xml
# virsh start pc-data
4. Create snapshot and check the data file XATTRs:
# virsh snapshot-create-as pc-data s1 --no-metadata --disk-only
# getfattr -m - -d /var/lib/libvirt/images/pc-data.raw
getfattr: Removing leading '/' from absolute path names
# file: var/lib/libvirt/images/pc-data.raw
security.selinux="unconfined_u:object_r:svirt_image_t:s0:c775,c1011"
trusted.libvirt.security.dac="+107:+107"
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.ref_selinux="1"
trusted.libvirt.security.selinux="unconfined_u:object_r:svirt_image_t:s0:c284,c367"
trusted.libvirt.security.timestamp_dac="1563328069"
trusted.libvirt.security.timestamp_selinux="1563328069"
Shutdown the VM. The XATTRs of data file is not changed.
It is not expected. The XTTRs should not contain *.libvirt.*
Issue is not reproduced with -blockdev enabled:
<domain type='kvm'
xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
...
<qemu:capabilities>
<qemu:add capability='blockdev'/>
<qemu:del capability='drive'/>
</qemu:capabilities>
</domain>
See the libvirt daemon log and vm xml in attachment.
On Tue, Oct 8, 2019 at 5:49 AM Cole Robinson <crobinso(a)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
src/libvirt_private.syms | 1 -
src/security/security_dac.c | 63 +++++--
src/security/security_selinux.c | 97 +++++++----
src/util/virstoragefile.c | 281 ++++++++++++++++++++------------
src/util/virstoragefile.h | 11 +-
5 files changed, 290 insertions(+), 163 deletions(-)
--
2.23.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan(a)redhat.com
Phone: +861065339333
12:04 p.m.
On 10/15/19 3:56 AM, Han Han wrote:
Hello Cole, one issue is found:
The qcow2 data file XTTRs is not cleaned on external snapshot when
-blockdev is not enabled
Versions:
libvirt v5.8.0-134-g9d03e9adf1
qemu-kvm-4.1.0-13.module+el8.1.0+4313+ef76ec61.x86_64
Steps:
1. Convert a OS image to qcow2&qcow2 data file:
# qemu-img convert -O qcow2 -o
data_file=/var/lib/libvirt/images/pc-data.raw,data_file_raw=on
/var/lib/libvirt/images/pc.qcow2 /var/lib/libvirt/images/pc-data.qcow2
2. Build and start libvirt source, start libvirt daemon:
# make clean && CC=/usr/lib64/ccache/cc ./autogen.sh&&./configure
--without-libssh --build=x86_64-redhat-linux-gnu
--host=x86_64-redhat-linux-gnu --program-prefix=
--disable-dependency-tracking --prefix=/usr --exec-prefix=/usr
--bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc
--datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64
--libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib
--mandir=/usr/share/man --infodir=/usr/share/info --with-qemu
--without-openvz --without-lxc --without-vbox --without-libxl
--with-sasl --with-polkit --with-libvirtd --without-phyp --with-esx
--without-hyperv --without-vmware --without-xenapi --without-vz
--without-bhyve --with-interface --with-network --with-storage-fs
--with-storage-lvm --with-storage-iscsi --with-storage-iscsi-direct
--with-storage-scsi --with-storage-disk --with-storage-mpath
--with-storage-rbd --without-storage-sheepdog --with-storage-gluster
--without-storage-zfs --without-storage-vstorage --with-numactl
--with-numad --with-capng --without-fuse --with-netcf --with-selinux
--with-selinux-mount=/sys/fs/selinux --without-apparmor --without-hal
--with-udev --with-yajl --with-sanlock --with-libpcap --with-macvtap
--with-audit --with-dtrace --with-driver-modules --with-firewalld
--with-firewalld-zone --without-wireshark-dissector --without-pm-utils
--with-nss-plugin '--with-packager=Unknown, 2019-08-19-12:13:01,
lab.rhel8.me <http://lab.rhel8.me>' --with-packager-version=1.el8
--with-qemu-user=qemu --with-qemu-group=qemu
--with-tls-priority=@LIBVIRT,SYSTEM --enable-werror
--enable-expensive-tests --with-init-script=systemd
--without-login-shell && make -j8
# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')"
src/.libs/virtlogd
# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')"
LIBVIRT_DEBUG=3
LIBVIRT_LOG_FILTERS="1:util 1:qemu 1:security"
LIBVIRT_LOG_OUTPUTS="1:file:/tmp/libvirt_daemon.log" src/.libs/libvirtd
3. Define and start an VM with the qcow2&qcow2 data file. Note that the
-blockdev is not enabled
# virsh define pc-data.xml
# virsh start pc-data
4. Create snapshot and check the data file XATTRs:
# virsh snapshot-create-as pc-data s1 --no-metadata --disk-only
# getfattr -m - -d /var/lib/libvirt/images/pc-data.raw
getfattr: Removing leading '/' from absolute path names
# file: var/lib/libvirt/images/pc-data.raw
security.selinux="unconfined_u:object_r:svirt_image_t:s0:c775,c1011"
trusted.libvirt.security.dac="+107:+107"
trusted.libvirt.security.ref_dac="1"
trusted.libvirt.security.ref_selinux="1"
trusted.libvirt.security.selinux="unconfined_u:object_r:svirt_image_t:s0:c284,c367"
trusted.libvirt.security.timestamp_dac="1563328069"
trusted.libvirt.security.timestamp_selinux="1563328069"
Shutdown the VM. The XATTRs of data file is not changed.
It is not expected. The XTTRs should not contain *.libvirt.*
Issue is not reproduced with -blockdev enabled:
<domain type='kvm'
xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
...
<qemu:capabilities>
<qemu:add capability='blockdev'/>
<qemu:del capability='drive'/>
</qemu:capabilities>
</domain>
See the libvirt daemon log and vm xml in attachment.
Nice catch! I will need to dig into this to figure out where the issue
is. Can you put this info into an upstream bug report in
product=Virtualization Tools and I will get to it when I can
Thanks,
Cole
9:28 p.m.
On Wed, Oct 16, 2019 at 1:04 AM Cole Robinson <crobinso(a)redhat.com> wrote:
On 10/15/19 3:56 AM, Han Han wrote:
> Hello Cole, one issue is found:
> The qcow2 data file XTTRs is not cleaned on external snapshot when
> -blockdev is not enabled
>
> Versions:
> libvirt v5.8.0-134-g9d03e9adf1
> qemu-kvm-4.1.0-13.module+el8.1.0+4313+ef76ec61.x86_64
>
> Steps:
> 1. Convert a OS image to qcow2&qcow2 data file:
> # qemu-img convert -O qcow2 -o
> data_file=/var/lib/libvirt/images/pc-data.raw,data_file_raw=on
> /var/lib/libvirt/images/pc.qcow2 /var/lib/libvirt/images/pc-data.qcow2
>
> 2. Build and start libvirt source, start libvirt daemon:
> # make clean && CC=/usr/lib64/ccache/cc ./autogen.sh&&./configure
> --without-libssh --build=x86_64-redhat-linux-gnu
> --host=x86_64-redhat-linux-gnu --program-prefix=
> --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr
> --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc
> --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64
> --libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib
> --mandir=/usr/share/man --infodir=/usr/share/info --with-qemu
> --without-openvz --without-lxc --without-vbox --without-libxl
> --with-sasl --with-polkit --with-libvirtd --without-phyp --with-esx
> --without-hyperv --without-vmware --without-xenapi --without-vz
> --without-bhyve --with-interface --with-network --with-storage-fs
> --with-storage-lvm --with-storage-iscsi --with-storage-iscsi-direct
> --with-storage-scsi --with-storage-disk --with-storage-mpath
> --with-storage-rbd --without-storage-sheepdog --with-storage-gluster
> --without-storage-zfs --without-storage-vstorage --with-numactl
> --with-numad --with-capng --without-fuse --with-netcf --with-selinux
> --with-selinux-mount=/sys/fs/selinux --without-apparmor --without-hal
> --with-udev --with-yajl --with-sanlock --with-libpcap --with-macvtap
> --with-audit --with-dtrace --with-driver-modules --with-firewalld
> --with-firewalld-zone --without-wireshark-dissector --without-pm-utils
> --with-nss-plugin '--with-packager=Unknown, 2019-08-19-12:13:01,
> lab.rhel8.me <http://lab.rhel8.me>' --with-packager-version=1.el8
> --with-qemu-user=qemu --with-qemu-group=qemu
> --with-tls-priority=@LIBVIRT,SYSTEM --enable-werror
> --enable-expensive-tests --with-init-script=systemd
> --without-login-shell && make -j8
> # LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' '
')" src/.libs/virtlogd
> # LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' '
')" LIBVIRT_DEBUG=3
> LIBVIRT_LOG_FILTERS="1:util 1:qemu 1:security"
> LIBVIRT_LOG_OUTPUTS="1:file:/tmp/libvirt_daemon.log" src/.libs/libvirtd
>
> 3. Define and start an VM with the qcow2&qcow2 data file. Note that the
> -blockdev is not enabled
> # virsh define pc-data.xml
> # virsh start pc-data
>
> 4. Create snapshot and check the data file XATTRs:
> # virsh snapshot-create-as pc-data s1 --no-metadata --disk-only
> # getfattr -m - -d /var/lib/libvirt/images/pc-data.raw
> getfattr: Removing leading '/' from absolute path names
> # file: var/lib/libvirt/images/pc-data.raw
> security.selinux="unconfined_u:object_r:svirt_image_t:s0:c775,c1011"
> trusted.libvirt.security.dac="+107:+107"
> trusted.libvirt.security.ref_dac="1"
> trusted.libvirt.security.ref_selinux="1"
>
trusted.libvirt.security.selinux="unconfined_u:object_r:svirt_image_t:s0:c284,c367"
> trusted.libvirt.security.timestamp_dac="1563328069"
> trusted.libvirt.security.timestamp_selinux="1563328069"
>
> Shutdown the VM. The XATTRs of data file is not changed.
> It is not expected. The XTTRs should not contain *.libvirt.*
>
> Issue is not reproduced with -blockdev enabled:
> <domain type='kvm' xmlns:qemu='
http://libvirt.org/schemas/domain/qemu/1.0'>
> ...
> <qemu:capabilities>
> <qemu:add capability='blockdev'/>
> <qemu:del capability='drive'/>
> </qemu:capabilities>
> </domain>
>
> See the libvirt daemon log and vm xml in attachment.
Nice catch! I will need to dig into this to figure out where the issue
is. Can you put this info into an upstream bug report in
Sure. https://bugzilla.redhat.com/show_bug.cgi?id=1762135
product=Virtualization Tools and I will get to it when I can
Thanks,
Cole
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan(a)redhat.com
Phone: +861065339333
4:26 a.m.
Cole Robinson writes:
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
s/is stores/stores/
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
What happens if you try to do that in place, i.e. EXISTING==NEW?
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
--
Cheers,
Christophe de Dinechin (IRC c3d)
7:35 a.m.
Hmm why wasn't I in to/cc list, was that intentional?
On 10/17/19 5:26 AM, Christophe de Dinechin wrote:
Cole Robinson writes:
> 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
s/is stores/stores/
Indeed, but what's the benefit of correcting grammar in a cover letter
for already applied patches?
> 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
What happens if you try to do that in place, i.e. EXISTING==NEW?
Results in an unbootable image in my testing. I'm guessing it
initializes the raw image first, similar to trying to use 'qemu-img
create' with an existing raw image as data_file
https://bugzilla.redhat.com/show_bug.cgi?id=1688814#c8
- Cole
7:50 a.m.
On 17 Oct 2019, at 14:35, Cole Robinson <crobinso(a)redhat.com>
wrote:
Hmm why wasn't I in to/cc list, was that intentional?
Not at all. I assumed a reply-to would add it automatically and did not
bother checking. Probably something wrong in my mu4e config.
On 10/17/19 5:26 AM, Christophe de Dinechin wrote:
>
> Cole Robinson writes:
>
>> 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
>
> s/is stores/stores/
Indeed, but what's the benefit of correcting grammar in a cover letter
for already applied patches?
Obviously, I realized you had already applied that patch only after sending
my email ;-) I saw that there was still some active discussion on that thread,
so I assumed it was still under review.
>
> What happens if you try to do that in place, i.e. EXISTING==NEW?
>
Results in an unbootable image in my testing. I'm guessing it
initializes the raw image first, similar to trying to use 'qemu-img
create' with an existing raw image as data_file
https://bugzilla.redhat.com/show_bug.cgi?id=1688814#c8
<https://bugzilla.redhat.com/show_bug.cgi?id=1688814#c8>
OK. Might be worth catching or fixing at some point.
Christophe
1862
days inactive
1872
days old
89 comments
7 participants
participants (7)
-
Christian Ehrhardt -
Christophe de Dinechin -
Cole Robinson -
Daniel Henrique Barboza -
Han Han -
Ján Tomko -
Michal Privoznik