Nir Soffer has posted comments on this change.

Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same 
subnet
......................................................................


Patch Set 17:

(1 comment)

http://gerrit.ovirt.org/#/c/31529/17/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 1136: 
Line 1137: 
Line 1138: def set_rp_filter(dev, mode):
Line 1139:     path = '/proc/sys/net/ipv4/conf/%s/rp_filter' % dev
Line 1140:     with open(path, 'w') as rp_filter:
> Why is buffering an issue in this case? (I don't believe that it is)
buffering is not an issue, this is just bloat. Check all the code involved in 
creating a file object and writing, when the needed operations are just 3 
syscalls.

I don't suggest to replace all open calls with this, but we can have an utility 
to write stuff to sysfs/procfs, or just open an fd in a context manager:

    @contextmanager
    def open_fd(path, options):
        fd = os.open(path, options)
        try:
            yield fd
        finally:
            os.close(fd)

Then writing efficient code is almost nice as using open:

    with open_fd(path, os.O_WRONLY) as fd:
        os.write(fd, data)

Not required now, just something for later.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf93d49317c76aece764e53e58e0ff28868f16b0
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amador Pahim <apa...@redhat.com>
Gerrit-Reviewer: Amador Pahim <apa...@redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: jian wang <wjia...@gmail.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to