11:07 a.m.
These two blocks exist in different storage backends' createVol
implementations:
if (vol->target.format != VIR_STORAGE_FILE_RAW) {
virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("only RAW volumes are supported by this storage
pool"));
return -VIR_ERR_NO_SUPPORT;
}
if (vol->target.encryption != NULL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("storage pool does not support encrypted
"
"volumes"));
return -1;
}
My hunch is that the second example (VIR_ERR_CONFIG_UNSUPPORTED and
returning -1) is correct in both of these cases.
Related... Which backends should be restricted to only raw? That code
is from storage_backend_rbd.c, which seems to be the only one
restricted in that way. But does it make sense to allow !raw on LVM (or
ZFS), for example?
Whatever the correct answer, I'm looking to submit one or more patches.
--
Richard
11:09 p.m.
Richard Laager wrote:
These two blocks exist in different storage backends' createVol
implementations:
if (vol->target.format != VIR_STORAGE_FILE_RAW) {
virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("only RAW volumes are supported by this storage
pool"));
return -VIR_ERR_NO_SUPPORT;
}
if (vol->target.encryption != NULL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("storage pool does not support encrypted
"
"volumes"));
return -1;
}
My hunch is that the second example (VIR_ERR_CONFIG_UNSUPPORTED and
returning -1) is correct in both of these cases.
I think the second example is more correct and
VIR_ERR_CONFIG_UNSUPPORTED should be used is such cases, because
VIR_ERR_NO_SUPPORT is supposed to mean a function unsupported by the
driver itself, not a situation when a user is requesting some
configuration that is not possible.
Related... Which backends should be restricted to only raw? That
code
is from storage_backend_rbd.c, which seems to be the only one
restricted in that way. But does it make sense to allow !raw on LVM (or
ZFS), for example?
As for ZFS, I don't see how non-raw could be used. I didn't add a check
for that because I just did not think about that. Not sure about the
other backends though.
Whatever the correct answer, I'm looking to submit one or more
patches.
--
Richard
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
2:17 p.m.
New subject: [libvirt] [PATCH 1/6] rbd: Use proper error type
---
src/storage/storage_backend_rbd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
index 7d04b39..3e70a42 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -671,9 +671,9 @@ virStorageBackendRBDCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
vol->type = VIR_STORAGE_VOL_NETWORK;
if (vol->target.format != VIR_STORAGE_FILE_RAW) {
- virReportError(VIR_ERR_NO_SUPPORT, "%s",
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("only RAW volumes are supported by this storage
pool"));
- return -VIR_ERR_NO_SUPPORT;
+ return -1;
}
VIR_FREE(vol->target.path);
--
2.1.4
2:17 p.m.
New subject: [libvirt] [PATCH 2/6] sheepdog: Use a consistent error message
This also reduces the number of strings to translate.
---
src/storage/storage_backend_sheepdog.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_sheepdog.c
b/src/storage/storage_backend_sheepdog.c
index a3bd78a..001d16f 100644
--- a/src/storage/storage_backend_sheepdog.c
+++ b/src/storage/storage_backend_sheepdog.c
@@ -237,8 +237,9 @@ virStorageBackendSheepdogCreateVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
virStorageVolDefPtr vol)
{
if (vol->target.encryption != NULL) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Sheepdog does not support encrypted volumes"));
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("storage pool does not support encrypted
"
+ "volumes"));
return -1;
}
--
2.1.4
2:17 p.m.
New subject: [libvirt] [PATCH 3/6] storage: Improve code consistency between backends
This improves the code consistency around freeing vol->target.path in
createVol implementations.
---
src/storage/storage_backend_logical.c | 2 --
src/storage/storage_backend_zfs.c | 6 +-----
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index ecbf430..90a194e 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -921,9 +921,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_BLOCK;
- /* A target path passed to CreateVol has no meaning */
VIR_FREE(vol->target.path);
-
if (virAsprintf(&vol->target.path, "%s/%s",
pool->def->target.path,
vol->name) == -1)
diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c
index 2e6e407..4129aae 100644
--- a/src/storage/storage_backend_zfs.c
+++ b/src/storage/storage_backend_zfs.c
@@ -303,11 +303,7 @@ virStorageBackendZFSCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
vol->type = VIR_STORAGE_VOL_BLOCK;
- if (vol->target.path != NULL) {
- /* A target path passed to CreateVol has no meaning */
- VIR_FREE(vol->target.path);
- }
-
+ VIR_FREE(vol->target.path);
if (virAsprintf(&vol->target.path, "%s/%s",
pool->def->target.path, vol->name) == -1)
return -1;
--
2.1.4
2:17 p.m.
New subject: [libvirt] [PATCH 4/6] logical: Only raw volumes are supported
---
src/storage/storage_backend_logical.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 90a194e..39e8b80 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -912,6 +912,12 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
struct stat sb;
bool created = false;
+ if (vol->target.format != VIR_STORAGE_FILE_RAW) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("only RAW volumes are supported by this storage
pool"));
+ return -1;
+ }
+
if (vol->target.encryption != NULL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("storage pool does not support encrypted
"
--
2.1.4
2:17 p.m.
New subject: [libvirt] [PATCH 5/6] zfs: Only raw volumes are supported
---
src/storage/storage_backend_zfs.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c
index 4129aae..077543d 100644
--- a/src/storage/storage_backend_zfs.c
+++ b/src/storage/storage_backend_zfs.c
@@ -301,6 +301,12 @@ virStorageBackendZFSCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
int ret = -1;
int volmode_needed = -1;
+ if (vol->target.format != VIR_STORAGE_FILE_RAW) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("only RAW volumes are supported by this storage
pool"));
+ return -1;
+ }
+
vol->type = VIR_STORAGE_VOL_BLOCK;
VIR_FREE(vol->target.path);
--
2.1.4
2:17 p.m.
New subject: [libvirt] [PATCH 6/6] zfs: Only unencrypted volumes are supported
---
src/storage/storage_backend_zfs.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c
index 077543d..5238ecc 100644
--- a/src/storage/storage_backend_zfs.c
+++ b/src/storage/storage_backend_zfs.c
@@ -307,6 +307,13 @@ virStorageBackendZFSCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
return -1;
}
+ if (vol->target.encryption != NULL) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("storage pool does not support encrypted
"
+ "volumes"));
+ return -1;
+ }
+
vol->type = VIR_STORAGE_VOL_BLOCK;
VIR_FREE(vol->target.path);
--
2.1.4
7:01 p.m.
New subject: [libvirt] [PATCH 1/6] rbd: Use proper error type
Richard Laager wrote:
---
src/storage/storage_backend_rbd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
index 7d04b39..3e70a42 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -671,9 +671,9 @@ virStorageBackendRBDCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
vol->type = VIR_STORAGE_VOL_NETWORK;
if (vol->target.format != VIR_STORAGE_FILE_RAW) {
- virReportError(VIR_ERR_NO_SUPPORT, "%s",
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("only RAW volumes are supported by this storage
pool"));
- return -VIR_ERR_NO_SUPPORT;
+ return -1;
}
VIR_FREE(vol->target.path);
ACK the series except the patch 4/6, because I'm not very familiar with
the logical backend (or lvm itself to be exact), so it would be great is
somebody could take a look at it.
Roman Bogorodskiy
2:25 p.m.
New subject: [libvirt] [PATCH 1/6] rbd: Use proper error type
Roman Bogorodskiy wrote:
Richard Laager wrote:
> ---
> src/storage/storage_backend_rbd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
> index 7d04b39..3e70a42 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -671,9 +671,9 @@ virStorageBackendRBDCreateVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
> vol->type = VIR_STORAGE_VOL_NETWORK;
>
> if (vol->target.format != VIR_STORAGE_FILE_RAW) {
> - virReportError(VIR_ERR_NO_SUPPORT, "%s",
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("only RAW volumes are supported by this storage
pool"));
> - return -VIR_ERR_NO_SUPPORT;
> + return -1;
> }
>
> VIR_FREE(vol->target.path);
ACK the series except the patch 4/6, because I'm not very familiar with
the logical backend (or lvm itself to be exact), so it would be great is
somebody could take a look at it.
As there were no comments on 4/6, ACK and pushed all the patches in the
series. Thanks!
Roman Bogorodskiy
3092
days inactive
3111
days old
9 comments
2 participants
participants (2)
-
Richard Laager -
Roman Bogorodskiy