From: Simon McVittie Date: Tue, 25 Jun 2013 11:56:28 +0000 (+0100) Subject: BmapCopy: downgrade inability to set sysfs parameters to a warning X-Git-Tag: v2.5~22 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=d96e613d35f173a8bc1058b8f033ce58db4f9034;p=tools%2Fbmap-tools.git BmapCopy: downgrade inability to set sysfs parameters to a warning On distributions where unprivileged users can write to removable USB disks, this allows such a disk to be written with bmaptool (albeit with non-optimal performance) without being root. Artem: got some more explanations from Simon McVittie about why we don't just add a special case for EPERM/EACCESS: """ > On 28/06/13 12:47, Artem Bityutskiy wrote: > For a library, requiring the user to have a logger object is probably > not the nicest thing. And usually libraries do not print error messages, > they throw exceptions instead. I made the logger optional: if the caller doesn't supply one, BmapCopy will use logging.getLogger(__name__), i.e. the logger named "bmaptools.BmapCopy", which appears to be best-practice for Python logging. (You could use it for debug-logging too, if you wanted to.) Having logger=None cause logging to be suppressed, instead of using the logging module's defaults (which are to print 'No handlers could be found for logger "foo.bar"' the first time you use it, and not log anything...) would also be fine. Libraries throw exceptions if they couldn't do what you asked, but I think there's some room for a middle ground between "no, I can't" and silent success. """ Fair enough, I think. Hoewver, I massged the patch a bit and improved the warnings to make them a bit more user-friendly and give users the idea what is the consequence of the warning. Change-Id: I644d9777b7bf9e97dbe002814f7d4e411a7618db Signed-off-by: Artem Bityutskiy --- diff --git a/bmaptools/BmapCopy.py b/bmaptools/BmapCopy.py index 211ce17..f7a4ace 100644 --- a/bmaptools/BmapCopy.py +++ b/bmaptools/BmapCopy.py @@ -565,25 +565,32 @@ class BmapBdevCopy(BmapCopy): f_scheduler.seek(0) f_scheduler.write("noop") except IOError as err: - raise Error("cannot enable the 'noop' I/O scheduler: %s" % err) - - # The file contains a list of scheduler with the current - # scheduler in square brackets, e.g., "noop deadline [cfq]". - # Fetch the current scheduler name - import re - - match = re.match(r'.*\[(.+)\].*', contents) - if match: - self._old_scheduler_value = match.group(1) - - # Limit the write buffering + self._logger.warning("failed to enable I/O optimization, expect " \ + "suboptimal speed (reason: cannot switch " \ + "to the 'noop' I/O scheduler: %s)" % err) + else: + # The file contains a list of schedulers with the current + # scheduler in square brackets, e.g., "noop deadline [cfq]". + # Fetch the name of the current scheduler. + import re + + match = re.match(r'.*\[(.+)\].*', contents) + if match: + self._old_scheduler_value = match.group(1) + + # Limit the write buffering, because we do not need too much of it when + # writing sequntially. Excessive buffering makes some systems not very + # responsive, e.g., this was observed in Fedora 17. try: with open(self._sysfs_max_ratio_path, "r+") as f_ratio: self._old_max_ratio_value = f_ratio.read() f_ratio.seek(0) f_ratio.write("1") except IOError as err: - raise Error("cannot set max. I/O ratio to '1': %s" % err) + self._logger.warning("failed to disable excessive buffering, " \ + "expect worse system responsiveness " \ + "(reason: cannot switch to the 'noop' " \ + "I/O scheduler: %s)" % err) def _restore_bdev_settings(self): """ Restore old block device settings which we changed in