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

[Martin Kletzander]

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

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

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 at redhat.com
>> https://listman.redhat.com/mailman/listinfo/libguestfs
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20210518/726fbc91/attachment.sig>