[node-patches] Change in ovirt-node[master]: utils_fs: Add symlink support

[asegurap at redhat.com]

Antoni Segura Puimedon has posted comments on this change.

Change subject: utils_fs: Add symlink support
......................................................................


Patch Set 6:

(4 comments)

Thanks for the review Douglas

http://gerrit.ovirt.org/#/c/29013/6/src/ovirt/node/utils/fs/__init__.py
File src/ovirt/node/utils/fs/__init__.py:

Line 24: """
Line 25: from __future__ import print_function
Line 26: 
Line 27: import shutil
Line 28: import errno
> please keep the alphabetic order.
There was no alphabetical order before:
    sors
now
    seors
Line 29: import os
Line 30: import re
Line 31: import StringIO
Line 32: 


Line 385: class Config(base.Base):
Line 386:     """oVirt Node specififc way to persist files
Line 387:     """
Line 388:     basedir = '/config'
Line 389:     path_entries = '/config/files'
> Isn't the case to use __init__ for these guys?
These are class constants, they are well placed. I don't see any reason to assign them on object initialization.
Line 390: 
Line 391:     def _config_path(self, fn=""):
Line 392:         return os.path.join(self.basedir, fn.strip("/"))
Line 393: 


Line 408:                     self._persist_dir(abspath)
Line 409:                 elif os.path.isfile(abspath):
Line 410:                     self._persist_file(abspath)
Line 411:             except Exception:
Line 412:                 self._logger.error('Failed to persist "%s"' % path)
> I am wondering if we need this return -1 at this point. Usually consuders u
I just did it to keep the old behavior. I will send a future patch that just raises here and have the calling code in scripts/persist do the catching and return an error code to the command line.
Line 413:                 return -1
Line 414: 
Line 415:     def _persist_dir(self, abspath):
Line 416:         """Persist directory and bind mount it back to its current location


Line 486: 
Line 487:     def _prepare_dir(self, abspath, persisted_path):
Line 488:         """Creates the necessary directory structure for persisted abspath
Line 489:         """
Line 490:         dir_path = os.path.dirname(persisted_path)
> what about:
It would be racy.
Line 491:         try:
Line 492:             os.makedirs(dir_path)
Line 493:         except OSError as ose:
Line 494:             if ose.errno != errno.EEXIST:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibcc52e9e5cd99bb7f342bb024d1aae12831ddc60
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes