[PATCH v2v v3 REBASE] rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279)
The validation helps us fail early and with a sensible error message. The NIL UUID is not valid for oVirt, but other than that there is no other logic in there merely because the UUID types are a matter of the generator and they are just forwarded in this partucular case. On top of that also check that the UUID is not already taken. This makes the code fail with a sensible error message instead of cryptic error from ovirtsdk. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v4: - https://listman.redhat.com/archives/libguestfs/2021-May/msg00095.html - Merge the two patches together - Things mentioned in the review are already in, so there is no other change v3: - https://listman.redhat.com/archives/libguestfs/2020-March/msg00084.html - Do the check in precheck - Fix for Lazy evaluation of regexp UUID v2: - https://www.redhat.com/archives/libguestfs/2020-January/msg00221.html - Use EEXIST instead of EINVAL - Put the colliding UUID into the error - Do not evaluate the PCRE needlessly multiple times v1: - https://www.redhat.com/archives/libguestfs/2020-January/msg00184.html v2v/output_rhv_upload.ml | 18 ++++++++++++++++++ v2v/rhv-upload-precheck.py | 10 ++++++++++ 2 files changed, 28 insertions(+) diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml index dbef7011b04b..15ba1078019e 100644 --- a/v2v/output_rhv_upload.ml +++ b/v2v/output_rhv_upload.ml @@ -49,6 +49,16 @@ after their uploads (if you do, you must supply one for each disk): -oo rhv-disk-uuid=UUID Disk UUID ") +let is_nonnil_uuid uuid = + let nil_uuid = "00000000-0000-0000-0000-000000000000" in + let rex_uuid = lazy ( + let hex = "[a-fA-F0-9]" in + let str = sprintf "^%s{8}-%s{4}-%s{4}-%s{4}-%s{12}$" hex hex hex hex hex in + PCRE.compile str + ) in + if uuid = nil_uuid then false + else PCRE.matches (Lazy.force rex_uuid) uuid + let parse_output_options options = let rhv_cafile = ref None in let rhv_cluster = ref None in @@ -71,6 +81,8 @@ let parse_output_options options = | "rhv-verifypeer", "" -> rhv_verifypeer := true | "rhv-verifypeer", v -> rhv_verifypeer := bool_of_string v | "rhv-disk-uuid", v -> + if not (is_nonnil_uuid v) then + error (f_"-o rhv-upload: invalid UUID for -oo rhv-disk-uuid"); rhv_disk_uuids := Some (v :: (Option.default [] !rhv_disk_uuids)) | k, _ -> error (f_"-o rhv-upload: unknown output option ‘-oo %s’") k @@ -256,6 +268,12 @@ object error_unless_output_alloc_sparse output_alloc; (* Python code prechecks. *) + let json_params = match rhv_options.rhv_disk_uuids with + | None -> json_params + | Some uuids -> + let ids = List.map (fun uuid -> JSON.String uuid) uuids in + ("rhv_disk_uuids", JSON.List ids) :: json_params + in let precheck_fn = tmpdir // "v2vprecheck.json" in let fd = Unix.openfile precheck_fn [O_WRONLY; O_CREAT] 0o600 in if Python_script.run_command ~stdout_fd:fd diff --git a/v2v/rhv-upload-precheck.py b/v2v/rhv-upload-precheck.py index 2180ea1043ab..c3f74df35f65 100644 --- a/v2v/rhv-upload-precheck.py +++ b/v2v/rhv-upload-precheck.py @@ -97,6 +97,16 @@ if cpu.architecture == types.Architecture.UNDEFINED: raise RuntimeError("The cluster ‘%s’ has an unknown architecture" % (params['rhv_cluster'])) +# Find if any disk already exists with specified UUID. +disks_service = system_service.disks_service() + +for uuid in params.get('rhv_disk_uuids', []): + try: + disk_service = disks_service.disk_service(uuid).get() + raise RuntimeError("Disk with the UUID '%s' already exists" % uuid) + except sdk.NotFoundError: + pass + # Otherwise everything is OK, print a JSON with the results. results = { "rhv_storagedomain_uuid": storage_domain.id, -- 2.31.1
On Tue, May 18, 2021 at 12:48 AM Martin Kletzander <mkletzan@redhat.com> wrote:
The validation helps us fail early and with a sensible error message. The NIL UUID is not valid for oVirt, but other than that there is no other logic in there merely because the UUID types are a matter of the generator and they are just forwarded in this partucular case.
particular
On top of that also check that the UUID is not already taken. This makes the code fail with a sensible error message instead of cryptic error from ovirtsdk.
Sounds good, ovirt sdk errors are not very helpful.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
v4: - https://listman.redhat.com/archives/libguestfs/2021-May/msg00095.html - Merge the two patches together - Things mentioned in the review are already in, so there is no other change v3: - https://listman.redhat.com/archives/libguestfs/2020-March/msg00084.html - Do the check in precheck - Fix for Lazy evaluation of regexp UUID v2: - https://www.redhat.com/archives/libguestfs/2020-January/msg00221.html - Use EEXIST instead of EINVAL - Put the colliding UUID into the error - Do not evaluate the PCRE needlessly multiple times v1: - https://www.redhat.com/archives/libguestfs/2020-January/msg00184.html
v2v/output_rhv_upload.ml | 18 ++++++++++++++++++ v2v/rhv-upload-precheck.py | 10 ++++++++++ 2 files changed, 28 insertions(+)
diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml index dbef7011b04b..15ba1078019e 100644 --- a/v2v/output_rhv_upload.ml +++ b/v2v/output_rhv_upload.ml @@ -49,6 +49,16 @@ after their uploads (if you do, you must supply one for each disk): -oo rhv-disk-uuid=UUID Disk UUID ")
+let is_nonnil_uuid uuid = + let nil_uuid = "00000000-0000-0000-0000-000000000000" in + let rex_uuid = lazy ( + let hex = "[a-fA-F0-9]" in + let str = sprintf "^%s{8}-%s{4}-%s{4}-%s{4}-%s{12}$" hex hex hex hex hex in + PCRE.compile str + ) in + if uuid = nil_uuid then false + else PCRE.matches (Lazy.force rex_uuid) uuid + let parse_output_options options = let rhv_cafile = ref None in let rhv_cluster = ref None in @@ -71,6 +81,8 @@ let parse_output_options options = | "rhv-verifypeer", "" -> rhv_verifypeer := true | "rhv-verifypeer", v -> rhv_verifypeer := bool_of_string v | "rhv-disk-uuid", v -> + if not (is_nonnil_uuid v) then + error (f_"-o rhv-upload: invalid UUID for -oo rhv-disk-uuid"); rhv_disk_uuids := Some (v :: (Option.default [] !rhv_disk_uuids)) | k, _ -> error (f_"-o rhv-upload: unknown output option ‘-oo %s’") k @@ -256,6 +268,12 @@ object error_unless_output_alloc_sparse output_alloc;
(* Python code prechecks. *) + let json_params = match rhv_options.rhv_disk_uuids with + | None -> json_params + | Some uuids -> + let ids = List.map (fun uuid -> JSON.String uuid) uuids in + ("rhv_disk_uuids", JSON.List ids) :: json_params + in let precheck_fn = tmpdir // "v2vprecheck.json" in let fd = Unix.openfile precheck_fn [O_WRONLY; O_CREAT] 0o600 in if Python_script.run_command ~stdout_fd:fd diff --git a/v2v/rhv-upload-precheck.py b/v2v/rhv-upload-precheck.py index 2180ea1043ab..c3f74df35f65 100644 --- a/v2v/rhv-upload-precheck.py +++ b/v2v/rhv-upload-precheck.py @@ -97,6 +97,16 @@ if cpu.architecture == types.Architecture.UNDEFINED: raise RuntimeError("The cluster ‘%s’ has an unknown architecture" % (params['rhv_cluster']))
+# Find if any disk already exists with specified UUID. +disks_service = system_service.disks_service() + +for uuid in params.get('rhv_disk_uuids', []): + try: + disk_service = disks_service.disk_service(uuid).get()
get() returns a disk, not a disk_service. You are overriding the disk_service instance with the result.
+ raise RuntimeError("Disk with the UUID '%s' already exists" % uuid) + except sdk.NotFoundError: + pass
Also this can be more clear using try-except-else: try: disks_service.disk_service(uuid).get() except sdk.NotFoundError: return results here? else: raise ...
+ # Otherwise everything is OK, print a JSON with the results. results = { "rhv_storagedomain_uuid": storage_domain.id, -- 2.31.1
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
On Tue, May 18, 2021 at 02:23:19PM +0300, Nir Soffer wrote:
On Tue, May 18, 2021 at 12:48 AM Martin Kletzander <mkletzan@redhat.com> wrote:
The validation helps us fail early and with a sensible error message. The NIL UUID is not valid for oVirt, but other than that there is no other logic in there merely because the UUID types are a matter of the generator and they are just forwarded in this partucular case.
particular
Good catch.
On top of that also check that the UUID is not already taken. This makes the code fail with a sensible error message instead of cryptic error from ovirtsdk.
Sounds good, ovirt sdk errors are not very helpful.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
v4: - https://listman.redhat.com/archives/libguestfs/2021-May/msg00095.html - Merge the two patches together - Things mentioned in the review are already in, so there is no other change v3: - https://listman.redhat.com/archives/libguestfs/2020-March/msg00084.html - Do the check in precheck - Fix for Lazy evaluation of regexp UUID v2: - https://www.redhat.com/archives/libguestfs/2020-January/msg00221.html - Use EEXIST instead of EINVAL - Put the colliding UUID into the error - Do not evaluate the PCRE needlessly multiple times v1: - https://www.redhat.com/archives/libguestfs/2020-January/msg00184.html
v2v/output_rhv_upload.ml | 18 ++++++++++++++++++ v2v/rhv-upload-precheck.py | 10 ++++++++++ 2 files changed, 28 insertions(+)
diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml index dbef7011b04b..15ba1078019e 100644 --- a/v2v/output_rhv_upload.ml +++ b/v2v/output_rhv_upload.ml @@ -49,6 +49,16 @@ after their uploads (if you do, you must supply one for each disk): -oo rhv-disk-uuid=UUID Disk UUID ")
+let is_nonnil_uuid uuid = + let nil_uuid = "00000000-0000-0000-0000-000000000000" in + let rex_uuid = lazy ( + let hex = "[a-fA-F0-9]" in + let str = sprintf "^%s{8}-%s{4}-%s{4}-%s{4}-%s{12}$" hex hex hex hex hex in + PCRE.compile str + ) in + if uuid = nil_uuid then false + else PCRE.matches (Lazy.force rex_uuid) uuid + let parse_output_options options = let rhv_cafile = ref None in let rhv_cluster = ref None in @@ -71,6 +81,8 @@ let parse_output_options options = | "rhv-verifypeer", "" -> rhv_verifypeer := true | "rhv-verifypeer", v -> rhv_verifypeer := bool_of_string v | "rhv-disk-uuid", v -> + if not (is_nonnil_uuid v) then + error (f_"-o rhv-upload: invalid UUID for -oo rhv-disk-uuid"); rhv_disk_uuids := Some (v :: (Option.default [] !rhv_disk_uuids)) | k, _ -> error (f_"-o rhv-upload: unknown output option ‘-oo %s’") k @@ -256,6 +268,12 @@ object error_unless_output_alloc_sparse output_alloc;
(* Python code prechecks. *) + let json_params = match rhv_options.rhv_disk_uuids with + | None -> json_params + | Some uuids -> + let ids = List.map (fun uuid -> JSON.String uuid) uuids in + ("rhv_disk_uuids", JSON.List ids) :: json_params + in let precheck_fn = tmpdir // "v2vprecheck.json" in let fd = Unix.openfile precheck_fn [O_WRONLY; O_CREAT] 0o600 in if Python_script.run_command ~stdout_fd:fd diff --git a/v2v/rhv-upload-precheck.py b/v2v/rhv-upload-precheck.py index 2180ea1043ab..c3f74df35f65 100644 --- a/v2v/rhv-upload-precheck.py +++ b/v2v/rhv-upload-precheck.py @@ -97,6 +97,16 @@ if cpu.architecture == types.Architecture.UNDEFINED: raise RuntimeError("The cluster ‘%s’ has an unknown architecture" % (params['rhv_cluster']))
+# Find if any disk already exists with specified UUID. +disks_service = system_service.disks_service() + +for uuid in params.get('rhv_disk_uuids', []): + try: + disk_service = disks_service.disk_service(uuid).get()
get() returns a disk, not a disk_service. You are overriding the disk_service instance with the result.
disk_service is an unused variable, but I agree I should've used another name. I guess it's a leftover (I haven't seen this patch in a year) and before it was just keeping the disk_service, maybe. Definitely `disk` is more appropriate name here. We need to call get() here, otherwise it only constructs the service and does not make any call to the remote side, which would not be helpful since we need to check if it exists.
+ raise RuntimeError("Disk with the UUID '%s' already exists" % uuid) + except sdk.NotFoundError: + pass
Also this can be more clear using try-except-else:
try: disks_service.disk_service(uuid).get() except sdk.NotFoundError: return results here? else: raise ...
I, personally, find this less readable, but have no problems changing it if that is the preference. Thanks! Martin
+ # Otherwise everything is OK, print a JSON with the results. results = { "rhv_storagedomain_uuid": storage_domain.id, -- 2.31.1
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
On Mon, May 17, 2021 at 11:47:17PM +0200, Martin Kletzander wrote:
The validation helps us fail early and with a sensible error message. The NIL UUID is not valid for oVirt, but other than that there is no other logic in there merely because the UUID types are a matter of the generator and they are just forwarded in this partucular case.
On top of that also check that the UUID is not already taken. This makes the code fail with a sensible error message instead of cryptic error from ovirtsdk.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
ACK, I will push it soon, thanks. Rich.
---
v4: - https://listman.redhat.com/archives/libguestfs/2021-May/msg00095.html - Merge the two patches together - Things mentioned in the review are already in, so there is no other change v3: - https://listman.redhat.com/archives/libguestfs/2020-March/msg00084.html - Do the check in precheck - Fix for Lazy evaluation of regexp UUID v2: - https://www.redhat.com/archives/libguestfs/2020-January/msg00221.html - Use EEXIST instead of EINVAL - Put the colliding UUID into the error - Do not evaluate the PCRE needlessly multiple times v1: - https://www.redhat.com/archives/libguestfs/2020-January/msg00184.html
v2v/output_rhv_upload.ml | 18 ++++++++++++++++++ v2v/rhv-upload-precheck.py | 10 ++++++++++ 2 files changed, 28 insertions(+)
diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml index dbef7011b04b..15ba1078019e 100644 --- a/v2v/output_rhv_upload.ml +++ b/v2v/output_rhv_upload.ml @@ -49,6 +49,16 @@ after their uploads (if you do, you must supply one for each disk): -oo rhv-disk-uuid=UUID Disk UUID ")
+let is_nonnil_uuid uuid = + let nil_uuid = "00000000-0000-0000-0000-000000000000" in + let rex_uuid = lazy ( + let hex = "[a-fA-F0-9]" in + let str = sprintf "^%s{8}-%s{4}-%s{4}-%s{4}-%s{12}$" hex hex hex hex hex in + PCRE.compile str + ) in + if uuid = nil_uuid then false + else PCRE.matches (Lazy.force rex_uuid) uuid + let parse_output_options options = let rhv_cafile = ref None in let rhv_cluster = ref None in @@ -71,6 +81,8 @@ let parse_output_options options = | "rhv-verifypeer", "" -> rhv_verifypeer := true | "rhv-verifypeer", v -> rhv_verifypeer := bool_of_string v | "rhv-disk-uuid", v -> + if not (is_nonnil_uuid v) then + error (f_"-o rhv-upload: invalid UUID for -oo rhv-disk-uuid"); rhv_disk_uuids := Some (v :: (Option.default [] !rhv_disk_uuids)) | k, _ -> error (f_"-o rhv-upload: unknown output option ‘-oo %s’") k @@ -256,6 +268,12 @@ object error_unless_output_alloc_sparse output_alloc;
(* Python code prechecks. *) + let json_params = match rhv_options.rhv_disk_uuids with + | None -> json_params + | Some uuids -> + let ids = List.map (fun uuid -> JSON.String uuid) uuids in + ("rhv_disk_uuids", JSON.List ids) :: json_params + in let precheck_fn = tmpdir // "v2vprecheck.json" in let fd = Unix.openfile precheck_fn [O_WRONLY; O_CREAT] 0o600 in if Python_script.run_command ~stdout_fd:fd diff --git a/v2v/rhv-upload-precheck.py b/v2v/rhv-upload-precheck.py index 2180ea1043ab..c3f74df35f65 100644 --- a/v2v/rhv-upload-precheck.py +++ b/v2v/rhv-upload-precheck.py @@ -97,6 +97,16 @@ if cpu.architecture == types.Architecture.UNDEFINED: raise RuntimeError("The cluster ‘%s’ has an unknown architecture" % (params['rhv_cluster']))
+# Find if any disk already exists with specified UUID. +disks_service = system_service.disks_service() + +for uuid in params.get('rhv_disk_uuids', []): + try: + disk_service = disks_service.disk_service(uuid).get() + raise RuntimeError("Disk with the UUID '%s' already exists" % uuid) + except sdk.NotFoundError: + pass + # Otherwise everything is OK, print a JSON with the results. results = { "rhv_storagedomain_uuid": storage_domain.id, -- 2.31.1
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
participants (3)
-
Martin Kletzander -
Nir Soffer -
Richard W.M. Jones