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

[Nir Soffer]

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