4:24 a.m.
The implementation was never finished properly. Since there isn't any
push to finish it either, let's remove the code so that it doesn't keep
rotting.
Peter Krempa (4):
security: Remove labelling of 'externalDataStore'
util: Remove 'externalDataStore' field from virStorageSource
util: Remove 'externalDataStoreRaw' field from virStorageSource
util: qcow2GetExtensions: Remove support for 'data file' extension
src/security/security_dac.c | 15 -------
src/security/security_selinux.c | 17 +-------
src/security/virt-aa-helper.c | 4 --
src/util/virstoragefile.c | 74 ++-------------------------------
src/util/virstoragefile.h | 5 ---
5 files changed, 5 insertions(+), 110 deletions(-)
--
2.26.0
4:24 a.m.
New subject: [PATCH 1/4] security: Remove labelling of 'externalDataStore'
The feature was never completed and is not really being pursued. Remove
the storage driver integration.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/security/security_dac.c | 15 ---------------
src/security/security_selinux.c | 17 +----------------
src/security/virt-aa-helper.c | 4 ----
3 files changed, 1 insertion(+), 35 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 11fff63bc7..bdc2d7edf3 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -983,14 +983,6 @@ virSecurityDACSetImageLabelRelative(virSecurityManagerPtr mgr,
if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 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;
@@ -1090,13 +1082,6 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr mgr,
if (virSecurityDACRestoreImageLabelSingle(mgr, def, src, migrated) < 0)
return -1;
- if (src->externalDataStore &&
- virSecurityDACRestoreImageLabelSingle(mgr,
- def,
- src->externalDataStore,
- migrated) < 0)
- return -1;
-
return 0;
}
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 281c303296..9a929debe1 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1806,13 +1806,6 @@ virSecuritySELinuxRestoreImageLabelInt(virSecurityManagerPtr mgr,
if (virSecuritySELinuxRestoreImageLabelSingle(mgr, def, src, migrated) < 0)
return -1;
- if (src->externalDataStore &&
- virSecuritySELinuxRestoreImageLabelSingle(mgr,
- def,
- src->externalDataStore,
- migrated) < 0)
- return -1;
-
return 0;
}
@@ -1880,7 +1873,7 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManagerPtr mgr,
return 0;
use_label = parent_seclabel->label;
- } else if (parent == src || parent->externalDataStore == src) {
+ } else if (parent == src) {
if (src->shared) {
use_label = data->file_context;
} else if (src->readonly) {
@@ -1942,14 +1935,6 @@ virSecuritySELinuxSetImageLabelRelative(virSecurityManagerPtr mgr,
if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent, isChainTop) <
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;
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 8526b7b985..6e6dd1b1db 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -898,10 +898,6 @@ storage_source_add_files(virStorageSourcePtr src,
if (add_file_path(tmp, depth, buf) < 0)
return -1;
- if (tmp->externalDataStore &&
- storage_source_add_files(tmp->externalDataStore, buf, depth) < 0)
- return -1;
-
depth++;
}
--
2.26.0
5:05 a.m.
New subject: [PATCH 1/4] security: Remove labelling of 'externalDataStore'
On a Friday in 2020, Peter Krempa wrote:
The feature was never completed and is not really being pursued.
Remove
the storage driver integration.
s/storage/security/
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/security/security_dac.c | 15 ---------------
src/security/security_selinux.c | 17 +----------------
src/security/virt-aa-helper.c | 4 ----
3 files changed, 1 insertion(+), 35 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:24 a.m.
New subject: [PATCH 2/4] util: Remove 'externalDataStore' field from virStorageSource
It's not used for anything so we don't need to extract it.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 41 ---------------------------------------
src/util/virstoragefile.h | 3 ---
2 files changed, 44 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index ffc8bdb344..69c354e7b4 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2475,12 +2475,6 @@ virStorageSourceCopy(const virStorageSource *src,
return NULL;
}
- if (src->externalDataStore) {
- if (!(def->externalDataStore =
virStorageSourceCopy(src->externalDataStore,
- true)))
- return NULL;
- }
-
/* ssh config passthrough for libguestfs */
def->ssh_host_key_check_disabled = src->ssh_host_key_check_disabled;
def->ssh_user = g_strdup(src->ssh_user);
@@ -2712,9 +2706,6 @@ virStorageSourceClear(virStorageSourcePtr def)
virStorageSourceSliceFree(def->sliceStorage);
- virObjectUnref(def->externalDataStore);
- def->externalDataStore = NULL;
-
virStorageNetHostDefFree(def->nhosts, def->hosts);
virStorageAuthDefFree(def->auth);
virObjectUnref(def->privateData);
@@ -4107,24 +4098,6 @@ 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
@@ -5351,20 +5324,6 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
return -1;
}
- if (src->externalDataStoreRaw) {
- g_autoptr(virStorageSource) externalDataStore = NULL;
-
- if ((rv = virStorageSourceNewFromExternalData(src,
- &externalDataStore)) < 0)
- return -1;
-
- /* the file would not be usable for VM usage */
- if (rv == 1)
- return 0;
-
- src->externalDataStore = g_steal_pointer(&externalDataStore);
- }
-
return 0;
}
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 7939c09cd5..8fe8e7c822 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -334,9 +334,6 @@ 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.26.0
5:05 a.m.
New subject: [PATCH 2/4] util: Remove 'externalDataStore' field from virStorageSource
On a Friday in 2020, Peter Krempa wrote:
It's not used for anything so we don't need to extract it.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 41 ---------------------------------------
src/util/virstoragefile.h | 3 ---
2 files changed, 44 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:24 a.m.
New subject: [PATCH 3/4] util: Remove 'externalDataStoreRaw' field from virStorageSource
It's not used for anything so we don't need to extract it.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 14 ++------------
src/util/virstoragefile.h | 2 --
2 files changed, 2 insertions(+), 14 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 69c354e7b4..32ca481cc0 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1022,12 +1022,6 @@ 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)
meta->compat = g_strdup("1.1");
@@ -2410,7 +2404,6 @@ virStorageSourceCopy(const virStorageSource *src,
def->relPath = g_strdup(src->relPath);
def->backingStoreRaw = g_strdup(src->backingStoreRaw);
def->backingStoreRawFormat = src->backingStoreRawFormat;
- def->externalDataStoreRaw = g_strdup(src->externalDataStoreRaw);
def->snapshot = g_strdup(src->snapshot);
def->configFile = g_strdup(src->configFile);
def->nodeformat = g_strdup(src->nodeformat);
@@ -2702,7 +2695,6 @@ virStorageSourceClear(virStorageSourcePtr def)
virStorageSourceSeclabelsClear(def);
virStoragePermsFree(def->perms);
VIR_FREE(def->timestamps);
- VIR_FREE(def->externalDataStoreRaw);
virStorageSourceSliceFree(def->sliceStorage);
@@ -5280,13 +5272,11 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
return -1;
/* If we probed the format we MUST ensure that nothing else than the current
- * image (this includes both backing files and external data store) is
- * considered for security labelling and/or recursion. */
+ * image is considered for security labelling and/or recursion. */
if (orig_format == VIR_STORAGE_FILE_AUTO) {
- if (src->backingStoreRaw || src->externalDataStoreRaw) {
+ if (src->backingStoreRaw) {
src->format = VIR_STORAGE_FILE_RAW;
VIR_FREE(src->backingStoreRaw);
- VIR_FREE(src->externalDataStoreRaw);
return -2;
}
}
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 8fe8e7c822..d56d5c45e3 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -345,8 +345,6 @@ struct _virStorageSource {
* current file. */
char *backingStoreRaw;
virStorageFileFormat backingStoreRawFormat;
- /* 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.26.0
5:05 a.m.
New subject: [PATCH 3/4] util: Remove 'externalDataStoreRaw' field from virStorageSource
On a Friday in 2020, Peter Krempa wrote:
It's not used for anything so we don't need to extract it.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 14 ++------------
src/util/virstoragefile.h | 2 --
2 files changed, 2 insertions(+), 14 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:24 a.m.
New subject: [PATCH 4/4] util: qcow2GetExtensions: Remove support for 'data file' extension
The implementation was never finished in libvirt. Remove it.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 19 ++-----------------
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 32ca481cc0..6dc9d1f016 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -199,7 +199,6 @@ 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)
@@ -426,8 +425,7 @@ cowGetBackingStore(char **res,
static int
qcow2GetExtensions(const char *buf,
size_t buf_size,
- int *backingFormat,
- char **externalDataStoreRaw)
+ int *backingFormat)
{
size_t offset;
size_t extension_start;
@@ -517,19 +515,6 @@ qcow2GetExtensions(const char *buf,
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;
- }
-
case QCOW2_HDR_EXTENSION_END:
return 0;
}
@@ -579,7 +564,7 @@ qcowXGetBackingStore(char **res,
memcpy(*res, buf + offset, size);
(*res)[size] = '\0';
- if (qcow2GetExtensions(buf, buf_size, format, NULL) < 0)
+ if (qcow2GetExtensions(buf, buf_size, format) < 0)
return BACKING_STORE_INVALID;
return BACKING_STORE_OK;
--
2.26.0
5:06 a.m.
New subject: [PATCH 4/4] util: qcow2GetExtensions: Remove support for 'data file' extension
On a Friday in 2020, Peter Krempa wrote:
The implementation was never finished in libvirt. Remove it.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 19 ++-----------------
1 file changed, 2 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
9:52 a.m.
New subject: [PATCH 4/4] util: qcow2GetExtensions: Remove support for 'data file' extension
On 4/24/20 4:24 AM, Peter Krempa wrote:
The implementation was never finished in libvirt. Remove it.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virstoragefile.c | 19 ++-----------------
1 file changed, 2 insertions(+), 17 deletions(-)
Shouldn't we at least recognize if there is a qcow2 file with the
extension header set, at which point we can error out and tell the user
their image is unsupported, rather than trying to use it without the
data file?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
10:18 a.m.
New subject: [PATCH 4/4] util: qcow2GetExtensions: Remove support for 'data file' extension
On Fri, Apr 24, 2020 at 09:52:00 -0500, Eric Blake wrote:
On 4/24/20 4:24 AM, Peter Krempa wrote:
> The implementation was never finished in libvirt. Remove it.
>
> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> src/util/virstoragefile.c | 19 ++-----------------
> 1 file changed, 2 insertions(+), 17 deletions(-)
Shouldn't we at least recognize if there is a qcow2 file with the extension
header set, at which point we can error out and tell the user their image is
unsupported, rather than trying to use it without the data file?
I don't think it's scalable to do so unless qemu exposes a whitelist of
supported extensions we can declare we support. Otherwise we are
band-aiding stuff we lear that exists and ignore any other extensions.
10:25 a.m.
New subject: [PATCH 4/4] util: qcow2GetExtensions: Remove support for 'data file' extension
On 4/24/20 10:18 AM, Peter Krempa wrote:
On Fri, Apr 24, 2020 at 09:52:00 -0500, Eric Blake wrote:
> On 4/24/20 4:24 AM, Peter Krempa wrote:
>> The implementation was never finished in libvirt. Remove it.
>>
>> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
>> ---
>> src/util/virstoragefile.c | 19 ++-----------------
>> 1 file changed, 2 insertions(+), 17 deletions(-)
>
> Shouldn't we at least recognize if there is a qcow2 file with the extension
> header set, at which point we can error out and tell the user their image is
> unsupported, rather than trying to use it without the data file?
I don't think it's scalable to do so unless qemu exposes a whitelist of
supported extensions we can declare we support. Otherwise we are
band-aiding stuff we lear that exists and ignore any other extensions.
external data files have an incompatible feature bit set in addition to
the header extension. Maybe whitelisting header extensions is
prohibitive, but we should be able to recognize when a qcow2 file has an
incompatible feature bit set that we are not aware of, and assume that
since we don't know about the feature, we also don't know how to safely
pass that file to qemu.
That is, we could quickly reject qcow2 files with offset 72-79
containing bit 2 set, until we are ready to support external data files
in libvirt.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
4:38 a.m.
New subject: [PATCH 4/4] util: qcow2GetExtensions: Remove support for 'data file' extension
On Fri, Apr 24, 2020 at 10:25:15 -0500, Eric Blake wrote:
On 4/24/20 10:18 AM, Peter Krempa wrote:
> On Fri, Apr 24, 2020 at 09:52:00 -0500, Eric Blake wrote:
> > On 4/24/20 4:24 AM, Peter Krempa wrote:
> > > The implementation was never finished in libvirt. Remove it.
> > >
> > > Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> > > ---
> > > src/util/virstoragefile.c | 19 ++-----------------
> > > 1 file changed, 2 insertions(+), 17 deletions(-)
> >
> > Shouldn't we at least recognize if there is a qcow2 file with the
extension
> > header set, at which point we can error out and tell the user their image is
> > unsupported, rather than trying to use it without the data file?
>
> I don't think it's scalable to do so unless qemu exposes a whitelist of
> supported extensions we can declare we support. Otherwise we are
> band-aiding stuff we lear that exists and ignore any other extensions.
external data files have an incompatible feature bit set in addition to the
header extension. Maybe whitelisting header extensions is prohibitive, but
we should be able to recognize when a qcow2 file has an incompatible feature
bit set that we are not aware of, and assume that since we don't know about
the feature, we also don't know how to safely pass that file to qemu.
That is, we could quickly reject qcow2 files with offset 72-79 containing
bit 2 set, until we are ready to support external data files in libvirt.
Are there any specific reasons to do so? If so we should track them in a
bug/issue in the first place. Additionally I'm against extending the
qcow2 header parser beyond what's strictly necessary. In the end our
reimplementation of of the parser is considered a security measure.
I've pushed the patches for now. If somebody will require the 'data
file' field the last patch can always be reverted.
1670
days inactive
1673
days old
12 comments
3 participants
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa