On Thu, Jan 23, 2020 at 11:12:22PM +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@redhat.com> --- v2v/output_rhv_upload.ml | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml index 14153db36897..923ef56d4ca5 100644 --- a/v2v/output_rhv_upload.ml +++ b/v2v/output_rhv_upload.ml @@ -49,6 +49,14 @@ 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 hex = "[a-fA-F0-9]" in + let rex_uuid = "^"^hex^"{8}-"^hex^"{4}-"^hex^"{4}-"^hex^"{4}-"^hex^"{12}$" in

Use sprintf?

+ let rex_uuid = PCRE.compile rex_uuid in + if uuid = nil_uuid then false + else PCRE.matches rex_uuid uuid

This could compile the regexp only once if you write this function as: let is_nonnil_uuid = let nil_uuid = "00000000-0000-0000-0000-000000000000" in let hex = "[a-fA-F0-9]" in let rex_uuid = sprintf "^%s{8}...." etc... in let rex_uuid = PCRE.compile rex_uuid in function uuid -> if uuid = nil_uuid then false else PCRE.matches rex_uuid uuid Or even better if you lazily evaluate the regexp so it is only compiled once when the function is first called: let is_nonnil_uuid = let rex_uuid = lazy ( let nil_uuid = "00000000-0000-0000-0000-000000000000" in let hex = "[a-fA-F0-9]" in let rex_uuid = sprintf "^%s{8}...." ... in PCRE.compile rex_uuid ) in function uuid -> if uuid = nil_uuid then false else PCRE.matches (Lazy.force rex_uuid) uuid But basically the idea is fine so ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/

On Thursday, 23 January 2020 23:12:23 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@redhat.com> --- 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..edb2699214c2 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:

'is not None' can be avoided. (Yes, there are few occurrences of that, should be changed too IMHO.)

+ try: + disks_service.disk_service(uuid).get() + nbdkit.set_error(errno.EINVAL)

Maybe EEXIST rather than EINVAL?

+ raise ValueError("Disk with the specified UUID already exists")

Print also the UUID, so it is easier to debug a failure even without the virt-v2v command line. With the above fixes, LGTM. -- Pino Toscano