Libguestfs - Jan 2020 - Re: [v2v PATCH 1/2] rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279)

My stab at fixing this: https://bugzilla.redhat.com/show_bug.cgi?id=1789279

It took me quite some time to go through the whole rfc 4122 just to realize we
do not need to do anything with the versions.

Martin Kletzander (2):
  rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279)
  rhv-upload: Check that rhv-disk-uuid is not already taken
    (RHBZ#1789279)

 v2v/output_rhv_upload.ml | 10 ++++++++++
 v2v/rhv-upload-plugin.py | 12 ++++++++++++
 2 files changed, 22 insertions(+)

-- 
2.25.0

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
+  let rex_uuid = PCRE.compile rex_uuid 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 +79,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
-- 
2.25.0

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:
+        try:
+            disks_service.disk_service(uuid).get()
+            nbdkit.set_error(errno.EINVAL)
+            raise ValueError("Disk with the specified UUID already
exists")
+        except sdk.NotFoundError:
+            pass
+
     if params['disk_format'] == "raw":
         disk_format = types.DiskFormat.RAW
     else:
-- 
2.25.0

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 Thu, Jan 23, 2020 at 11:12:23PM +0100, 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:
> +        try:
> +            disks_service.disk_service(uuid).get()
> +            nbdkit.set_error(errno.EINVAL)
> +            raise ValueError("Disk with the specified UUID already
exists")
> +        except sdk.NotFoundError:
> +            pass
> +
>      if params['disk_format'] == "raw":
>          disk_format = types.DiskFormat.RAW
>      else:
ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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