Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic mountpoint detection -- new version #54

Conversation

vojtechtrefny
Copy link
Member

New version of previous pull request -- #47

Changes:

  • MountsCache is now a singleton
  • It is not neccessary to add devices to cache from devicetree
  • Opening files using "with"
  • One line return in MountsCache.getMountpoint
  • tests

Signed-off-by: Vojtech Trefny <vtrefny@redhat.com>
Signed-off-by: Vojtech Trefny <vtrefny@redhat.com>
@@ -1604,14 +1615,14 @@ def _setOptions(self, options):

@property
def free(self):
if self._mountpoint:
# If self._mountpoint is defined, it means this tmpfs mount
if self.systeMountpoint:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watch out for spelling mistakes!..."systeMountpoint". pylint will definitely catch these kinds of things.

@mulkieran
Copy link
Contributor

Other than minor inline comments, that looks good to me. And a very nice improvement to our filesystem code, too.

@mulkieran mulkieran added the ACK label Mar 18, 2015
Current format._mountpoint attribute for "active" mountpoints is
being replaced with property format.systemMountpoint that returns
current mountpoint based on system information (cached information
from /proc/mounts and /proc/self/mountinfo)

Signed-off-by: Vojtech Trefny <vtrefny@redhat.com>
Handling nodev filesystems was originally part of getActiveMounts
method that is no longer needed.

Signed-off-by: Vojtech Trefny <vtrefny@redhat.com>
Signed-off-by: Vojtech Trefny <vtrefny@redhat.com>
Tests for new dynamic mountpoint detection using MountsCache

Signed-off-by: Vojtech Trefny <vtrefny@redhat.com>
@vojtechtrefny
Copy link
Member Author

I made few changes based on comments by mulkieran and bcl but after force pushing them most of the comments disappeared...

@mulkieran
Copy link
Contributor

Still looks good to me. FWIW, I ran a quick profile on the md5_file() method on my /proc/mounts and it's the read() method that chews up the bulk of the time, with updating the hash a distant second. And if we check whether there has been a change this way we just can't get away from reading those bits. Reading it in smaller chunks makes no difference...reading it in one big step doubles the time it takes.
Hoping that under normal use this change makes no perceptible difference (fingers crossed).

@vpodzime
Copy link
Contributor

Other than the neat-pick above this still looks good to me.

@vojtechtrefny
Copy link
Member Author

Docstring added. Pushed.

@vojtechtrefny vojtechtrefny deleted the master-mountpoints-new branch March 25, 2015 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants