2013/2/7 Stratos Psomadakis <[email protected]>:
> 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
You are right of course—I didn't think about that. It's fine if you
check for TypeError/ValueError.
>>> + # 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.
> 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.
Thanks!
> 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)?
Theoretically yes, but adding a bunch of files to test/data/ doesn't
hurt either. Adding a new directory will require more changes in
Makefile.am than just adding the files to TEST_FILES.
Michael
