9:54 a.m.
My stab v3 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.
v3:
- 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
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 | 18 ++++++++++++++++++
v2v/rhv-upload-precheck.py | 10 ++++++++++
2 files changed, 28 insertions(+)
--
2.25.1
9:54 a.m.
New subject: [PATCH v2v v3 1/2] rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279)
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>
---
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 9e60d8c73150..e833569318b3 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
--
2.25.1
5:19 p.m.
New subject: [PATCH v2v v3 1/2] rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279)
On Wed, Mar 11, 2020 at 03:54:33PM +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>
---
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 9e60d8c73150..e833569318b3 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
--
It needs to document the new option in docs/virt-v2v-output-rhv.pod
and in v2v/output_rhv_upload.ml in the print_output_options ()
function a little bit earlier in the file.
Apart from this the patch series is fine. Personally I think I would
have jammed the two patches into one, not sure I follow why they're
separate, but that's just a style thing.
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/
3:18 a.m.
New subject: [PATCH v2v v3 1/2] rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279)
On Wed, Mar 11, 2020 at 03:54:33PM +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>
---
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 9e60d8c73150..e833569318b3 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
Actually the use of lazy here is wrong. The regular expression
is not being created lazily at all.
You need to do something like:
let is_nonnil_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
fun uuid ->
if uuid = nil_uuid then false
else PCRE.matches (Lazy.force rex_uuid) uuid
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/
9:54 a.m.
New subject: [PATCH v2v v3 2/2] rhv-upload: Check that rhv-disk-uuid is not already taken (RHBZ#1789279)
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>
---
v2v/output_rhv_upload.ml | 6 ++++++
v2v/rhv-upload-precheck.py | 10 ++++++++++
2 files changed, 16 insertions(+)
diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml
index e833569318b3..c9fce5b95ec6 100644
--- a/v2v/output_rhv_upload.ml
+++ b/v2v/output_rhv_upload.ml
@@ -262,6 +262,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 ec3fcf4e5c20..0daa485e073b 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()
+ raise RuntimeError("Disk with the UUID '%s' already exists" %
uuid)
+ except sdk.NotFoundError:
+ pass
+
# Otherwise everything is OK, print a JSON with the results.
results = {
"rhv_storagedomain_uuid": storage_domain.id,
--
2.25.1
1715
days inactive
1716
days old
4 comments
2 participants
participants (2)
-
Martin Kletzander -
Richard W.M. Jones