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

nsoffer at redhat.com nsoffer at redhat.com
Mon Jun 9 15:04:18 UTC 2014


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,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib03152b852294f7b69a8734c2aa1206ea2c1fabe
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list