On 02/07/2013 12:37 PM, Michael Hanselmann wrote:
> 2013/2/5 Stratos Psomadakis <[email protected]>:
>> --- a/lib/bdev.py
>> +++ b/lib/bdev.py
>> @@ -38,11 +38,20 @@ from ganeti import objects
>> from ganeti import compat
>> from ganeti import netutils
>> from ganeti import pathutils
>> +from ganeti import serializer
>>
>> +from simplejson import JSONDecodeError
> If you need to detect errors, please do so in serializer.py by raising
> a custom exception from there (define it in errors.py). If
> serializer.py were to change its implementation this code here
> wouldn't work anymore as expected. You could name the exception class
> “JsonDecodeError”.
About this, other users of ganeti.serializer (eg lib/luxi.py), when
using LoadJson, they try to catch either an Exception or a ValueError
(parent class of simplejson.JSONDecodeError, and the exception that the
json module raises when given invalid input for loads()).
So, is it ok to also catch a ValueError exception for LoadJson() in
bdev.py, instead of adding a new exception / error?
If I add a class JsonDecodeError(GenericError) in errors.py, and raise
it from ganeti.serializer.LoadJson(), code that checks for ValueError
will need to be fixed (unless it's derived from the ValueError exception
class of course, but every other exception class in errors.py uses
GenericError, so I'm not sure if it's ok to do that).q
<snip>
>> if not splitted_lines:
>> - _ThrowError("rbd showmapped returned empty output")
>> + return None
>>
>> # Check showmapped header line, to determine number of fields.
>> field_cnt = len(splitted_lines[0])
>> if field_cnt != allfields:
>> - _ThrowError("Cannot parse rbd showmapped output because its format"
>> - " seems to have changed; expected %s fields, found %s",
>> - allfields, field_cnt)
>> + # Parsing the new format failed. Fallback to parsing the old output
>> + # format (< 0.55).
>> + splitted_lines = map(lambda l: l.split("\t"), lines)
>> + if field_cnt != allfields:
>> + _ThrowError("Cannot parse rbd showmapped output expected %s fields,"
>> + " found %s", allfields, field_cnt)
> Okay, can we please have unittests? There are just too many variations
> and version dependencies.
>
> Michael
Actually, this patch breaks the existing RADOSBlockDevice unit tests,
and I should have fixed them before sending the patch. :/ I'll fix them
and add tests for all three rbd output formats.
Btw, about the tests' data / input, would it be ok to add them as
separate files under test/data/bdev-rbd/ (the data for the current rbd
unit tests is in the py test file)?
Thanks,
Stratos
--
Stratos Psomadakis
<[email protected]>
