[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH v2v v3 REBASE] rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279)

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.


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>

 - 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
 - https://listman.redhat.com/archives/libguestfs/2020-March/msg00084.html
 - Do the check in precheck
 - Fix for Lazy evaluation of regexp UUID
 - 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
 - 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" %

+# 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:

except sdk.NotFoundError:
   return results here?
   raise ...

I, personally, find this less readable, but have no problems changing it
if that is the preference.


 # Otherwise everything is OK, print a JSON with the results.
 results = {
     "rhv_storagedomain_uuid": storage_domain.id,

Libguestfs mailing list
Libguestfs redhat com

Attachment: signature.asc
Description: PGP signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]