On Wed, Jan 29, 2020 at 03:34:48PM +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(a)redhat.com&gt; >--- > 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 14153db36897..6482460f8de8 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 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 I was too fast with the sending, consider this squashed in, (the fixed version is in my github repo [1]): diff --git i/v2v/output_rhv_upload.ml w/v2v/output_rhv_upload.ml index 6482460f8de8..c06c4c1844de 100644 --- i/v2v/output_rhv_upload.ml +++ w/v2v/output_rhv_upload.ml @@ -57,7 +57,7 @@ let is_nonnil_uuid uuid = PCRE.compile str ) in if uuid = nil_uuid then false - else PCRE.matches rex_uuid uuid + else PCRE.matches (Lazy.force rex_uuid) uuid let parse_output_options options = let rhv_cafile = ref None in --

This makes the code fail with a sensible error message instead of cryptic error from ovirtsdk. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- v2v/rhv-upload-plugin.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py index d3e6260e97f4..413ad53b05ab 100644 --- a/v2v/rhv-upload-plugin.py +++ b/v2v/rhv-upload-plugin.py @@ -26,6 +26,9 @@ import ssl import sys import time +import nbdkit +import errno + from http.client import HTTPSConnection, HTTPConnection from urllib.parse import urlparse @@ -461,6 +464,15 @@ def create_disk(connection): system_service = connection.system_service() disks_service = system_service.disks_service() + uuid = params.get('rhv_disk_uuid') + if uuid is not None: + try: + disks_service.disk_service(uuid).get() + nbdkit.set_error(errno.EEXIST) + raise ValueError("Disk with the UUID '%s' already exists" % uuid) + except sdk.NotFoundError: + pass

On Mon, Feb 24, 2020 at 10:28:46AM +0100, Pino Toscano wrote: >On Wednesday, 29 January 2020 15:34:49 CET Martin Kletzander wrote: >> This makes the code fail with a sensible error message instead of cryptic error >> from ovirtsdk. >> >> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; >> --- >> v2v/rhv-upload-plugin.py | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py >> index d3e6260e97f4..413ad53b05ab 100644 >> --- a/v2v/rhv-upload-plugin.py >> +++ b/v2v/rhv-upload-plugin.py >> @@ -26,6 +26,9 @@ import ssl >> import sys >> import time >> >> +import nbdkit >> +import errno >> + >> from http.client import HTTPSConnection, HTTPConnection >> from urllib.parse import urlparse >> >> @@ -461,6 +464,15 @@ def create_disk(connection): >> system_service = connection.system_service() >> disks_service = system_service.disks_service() >> >> + uuid = params.get('rhv_disk_uuid') >> + if uuid is not None: >> + try: >> + disks_service.disk_service(uuid).get() >> + nbdkit.set_error(errno.EEXIST) >> + raise ValueError("Disk with the UUID '%s' already exists" % uuid) >> + except sdk.NotFoundError: >> + pass > >This check seems correct to me, although it is done too late: IMHO this >is something to do in the precheck script, so we do not even attempt to >connect to the source if any of the specified UUIDs already exists in >oVirt. > Well at that point we are not passing the UUIDs to the script and that's why I selected this one (if I remember correctly).

Given how often this would happen I don't think it would be that big of a deal to reject it slightly slower.

Also the race window (of the UUID being created in the meantime) would become bigger.