thr3ads.net - Libguestfs - [Libguestfs] [PATCH v2v v3 REBASE] rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279) [May 2021]

If this information is useful, please help other people find it:
Share via:

Martin Kletzander

2021-May-17 21:47 UTC

[Libguestfs] [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 at 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

Nir Soffer

2021-May-18 11:23 UTC

head link

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

On Tue, May 18, 2021 at 12:48 AM Martin Kletzander <mkletzan at
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 at 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 at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs

Richard W.M. Jones

2021-Jun-03 11:01 UTC

head link

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

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 at 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

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

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

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

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