Change in vdsm[master]: volume: support multi line metadata values

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: volume: support multi line metadata values
......................................................................

Patch Set 3: Code-Review-1

(2 comments)

Too complex, no tests for very easy to test code.

http://gerrit.ovirt.org/#/c/28493/3/vdsm/storage/volume.py
File vdsm/storage/volume.py:

Line 823:             out[key.strip()] = "\n".join(value).strip()
Line 824: 
Line 825:         for l in meta:
Line 826:             if l.startswith("EOF"):
Line 827:                 break
This change is good but not required for this patch.
Line 828:             if l.find("=") < 0:
Line 829:                 value.append(l)
Line 830:                 continue
Line 831:             if key:

Line 833:             key, value = l.split("=")
Line 834:             value = [value]
Line 835: 
Line 836:         put(key, value)
Line 837:         return out
This is too complex.

You only need to change lines 823-825:

if l.find("=") < 0:
        if key is not None:
            out[key] += l # Continuation
    key, value = [s.strip() for s in l.split("=", 1)]
    out[key] = value

In any case - this code need tests, to make sure we did not break the old code, and that your new code actually works. While this is quite simple parser, it is too easy to make stupid errors in parsing. And it is trivial to write some tests for this. For example:

def test_empty(self):
        out = metadata2dict([])
        self.assertEquals({}, out}

def test_single_line(self):
        out = metadata2dict([
            'key 1 = value 1\n'
            'key 2 = value 2\n'
        ])
        self.assertEquals({'key 2': 'value 2', 'key 1': 'value 1'}, out}

With such easy to test code, there should be no excuse not to add tests.

Please add a patch with tests covering the old code behavior before this patch. Then it is trivial to see that your new code did not break the existing tests.
Line 838: 
Line 839:     def metadata2info(self, meta):
Line 840:         return {
Line 841:             "uuid": self.volUUID,