Change in vdsm[master]: vdsm/storage/misc.py: Convert uuid input to string before va...

[Ryan Harper]

Ryan Harper has uploaded a new change for review.

Change subject: vdsm/storage/misc.py: Convert uuid input to string before validating
......................................................................

vdsm/storage/misc.py: Convert uuid input to string before validating

When abusing the createVolume() method by passing numeric values
as UUIDs, the validation regex needs a string to process and
throws an exception when it isn't:

Traceback (most recent call last):
  File "/usr/share/vdsm/storage/task.py", line 861, in _run
    return fn(*args, **kargs)
  File "/usr/share/vdsm/logUtils.py", line 38, in wrapper
    res = f(*args, **kwargs)
  File "/usr/share/vdsm/storage/hsm.py", line 1206, in createVolume
    misc.validateUUID(imgUUID, 'imgUUID')
  File "/usr/share/vdsm/storage/misc.py", line 525, in validateUUID
    m = UUID_REGEX.match(uuid)
TypeError: expected string or buffer

And on the client side, I get a less than useful error:

Traceback (most recent call last):
  File "create_a_vm.py", line 98, in <module>
    BLANK_UUID, BLANK_UUID))
  File "create_a_vm.py", line 19, in vdsOK
    raise Exception(str(d))
Exception: {'status': {'message': "Error creating a new volume:
('sdUUID=99d43d50-5946-4451-8e12-9d85cb426e19,
spUUID=150e55ab-e68c-411c-af30-587071905216, imgUUID=4194304, size=4, volFormat=2,
preallocate=rootfs, diskType=7bdf7b4b-4909-41ce-a9c0-9bc06ba045fe,
volUUID=00000000-0000-0000-0000-000000000000, desc=00000000-0000-0000-0000-000000000000,
srcImgUUID=00000000-0000-0000-0000-000000000000,
srcVolUUID=00000000-0000-0000-0000-000000000000',)", 'code': 205}}

However, if we convert the input to a string first then we can
fail the REGEX and raise the more useful invalid parameter exception:

Now, I get:

Traceback (most recent call last):
  File "create_a_vm.py", line 98, in <module>
    BLANK_UUID, BLANK_UUID))
  File "create_a_vm.py", line 19, in vdsOK
    raise Exception(str(d))
Exception: {'status': {'message': "Invalid parameter:
'imgUUID=4194304'", 'code': 1000}}

Which explains to me my error quite well.

Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Signed-off-by: Ryan Harper <ryanh(a)us.ibm.com&gt;
---
M vdsm/storage/misc.py
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/87/3687/1
--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


Patch Set 4: Verified; Looks good to me, approved

this should not have been left out when adding the tests!

(this is the first time that cherry-pick has bitten us)

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


Patch Set 4: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

[Ryan Harper]

Ryan Harper has posted comments on this change.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/misc.py
Line 490:     if isinstance(uuid, str) or isinstance(uuid, unicode):
New to python "standards".  There's a whopping one line in the entire code
base that has this form for string type checking, so it's not like it was beating me
over the head with the exact form that this check should take.

In any case, it's easy enough to fix.

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


Patch Set 3: I would prefer that you didn't submit this

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


Patch Set 3: (1 inline comment)

-1 for visibility, code is fine

....................................................
File vdsm/storage/misc.py
Line 490:     if isinstance(uuid, str) or isinstance(uuid, unicode):
that's not the end of the world; I just wonder why you insistently avoid the standard

 isinstance(uuid, basestr)

check.

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

[Ryan Harper]

Ryan Harper has posted comments on this change.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/misc.py
Line 493:             return True
I see it now.  Let me resubmit and add an Integer test.

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/misc.py
Line 493:             return True
...and a utf-8-encoded Chinese test, if possible.

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


Patch Set 2: Fails

I now see that testInvalidInputNotHex and testWrongLength tests fail.

You may want to add another test for the integer input, so that preferred behavior is
maintained in the future.

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/storage/misc.py
Line 490:     if isinstance(uuid, str) or isinstance(uuid, unicode):
isinstance(uuid, basestring)

is the "standard" test

Line 493:             return True
but more importantly, now you return None instead of raising.

I suppose you meant to drop the "else" below.

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

Lei Li has posted comments on this change.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


Patch Set 2: Verified; Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm/storage/misc.py: Convert uuid input to string before validating
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

....................................................
Commit Message
Line 45: Which explains to me my error quite well.
However, if the function now receives utf-8-encoded buffer for uuid, it would now explode
instead of returning false.

I prefer that you validate that uuid is an instance of BaseString instead of casting (even
though Python purist would reject even that, together with all validate.* functions)

--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

Change in vdsm[master]: vdsm/storage/misc.py: ensure uuid input is str or unicode fo...

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vdsm/storage/misc.py: ensure uuid input is str or unicode for regex
......................................................................


vdsm/storage/misc.py: ensure uuid input is str or unicode for regex

When abusing the createVolume() method by passing numeric values
as UUIDs, the validation regex needs a string to process and
throws an exception when it isn't:

Traceback (most recent call last):
  File "/usr/share/vdsm/storage/task.py", line 861, in _run
    return fn(*args, **kargs)
  File "/usr/share/vdsm/logUtils.py", line 38, in wrapper
    res = f(*args, **kwargs)
  File "/usr/share/vdsm/storage/hsm.py", line 1206, in createVolume
    misc.validateUUID(imgUUID, 'imgUUID')
  File "/usr/share/vdsm/storage/misc.py", line 525, in validateUUID
    m = UUID_REGEX.match(uuid)
TypeError: expected string or buffer

And on the client side, I get a less than useful error:

Traceback (most recent call last):
  File "create_a_vm.py", line 98, in <module>
    BLANK_UUID, BLANK_UUID))
  File "create_a_vm.py", line 19, in vdsOK
    raise Exception(str(d))
Exception: {'status': {'message': "Error creating a new volume:
('sdUUID=99d43d50-5946-4451-8e12-9d85cb426e19,
spUUID=150e55ab-e68c-411c-af30-587071905216, imgUUID=4194304, size=4, volFormat=2,
preallocate=rootfs, diskType=7bdf7b4b-4909-41ce-a9c0-9bc06ba045fe,
volUUID=00000000-0000-0000-0000-000000000000, desc=00000000-0000-0000-0000-000000000000,
srcImgUUID=00000000-0000-0000-0000-000000000000,
srcVolUUID=00000000-0000-0000-0000-000000000000',)", 'code': 205}}

We can test that the input is str or unicode before attempting to regex
if it's something else, raise an exception.

Now, I get:

Traceback (most recent call last):
  File "create_a_vm.py", line 98, in <module>
    BLANK_UUID, BLANK_UUID))
  File "create_a_vm.py", line 19, in vdsOK
    raise Exception(str(d))
Exception: {'status': {'message': "Invalid parameter:
'imgUUID=4194304'", 'code': 1000}}

Which explains to me my error quite well.

V2 -> V3:
    - change check to use isinstance(s, basestring) per Dan Kenigsberg
V1 -> V2:
    - Fix case where we pass in a string, but it's not a UUID as
      Reported by Dan Kenigsberg <danken(a)redhat.com&gt;

Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Signed-off-by: Ryan Harper <ryanh(a)us.ibm.com&gt;
---
M vdsm/storage/misc.py
1 file changed, 6 insertions(+), 4 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved


--
To view, visit http://gerrit.ovirt.org/3687
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: If16466e88a50d3af03ae7830d4d8148ebf69d0d9
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ryan Harper <ryanh(a)us.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com&gt;