[PATCH v2v v3 0/2] rhv-upload: Validate UUIDs and check they don't exist already
My stab v3 at fixing this: https://bugzilla.redhat.com/show_bug.cgi?id=1789279 It took me quite some time to go through the whole rfc 4122 just to realize we do not need to do anything with the versions. v3: - 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 Martin Kletzander (2): rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279) rhv-upload: Check that rhv-disk-uuid is not already taken (RHBZ#1789279) v2v/output_rhv_upload.ml | 18 ++++++++++++++++++ v2v/rhv-upload-precheck.py | 10 ++++++++++ 2 files changed, 28 insertions(+) -- 2.25.1
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. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2v/output_rhv_upload.ml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml index 9e60d8c73150..e833569318b3 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 -- 2.25.1
On Wed, Mar 11, 2020 at 03:54:33PM +0100, 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.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2v/output_rhv_upload.ml | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml index 9e60d8c73150..e833569318b3 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 --
It needs to document the new option in docs/virt-v2v-output-rhv.pod and in v2v/output_rhv_upload.ml in the print_output_options () function a little bit earlier in the file. Apart from this the patch series is fine. Personally I think I would have jammed the two patches into one, not sure I follow why they're separate, but that's just a style thing. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
On Wed, Mar 11, 2020 at 03:54:33PM +0100, 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.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2v/output_rhv_upload.ml | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml index 9e60d8c73150..e833569318b3 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
Actually the use of lazy here is wrong. The regular expression is not being created lazily at all. You need to do something like: let is_nonnil_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 fun uuid -> if uuid = nil_uuid then false else PCRE.matches (Lazy.force rex_uuid) uuid Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
This makes the code fail with a sensible error message instead of cryptic error from ovirtsdk. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2v/output_rhv_upload.ml | 6 ++++++ v2v/rhv-upload-precheck.py | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml index e833569318b3..c9fce5b95ec6 100644 --- a/v2v/output_rhv_upload.ml +++ b/v2v/output_rhv_upload.ml @@ -262,6 +262,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 ec3fcf4e5c20..0daa485e073b 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.25.1
participants (2)
-
Martin Kletzander -
Richard W.M. Jones