[libvirt] Storage Backend Error Handling Consistency
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
--- 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
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
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
--- 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
--- 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
--- 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
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
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
participants (2)
-
Richard Laager -
Roman Bogorodskiy