[PATCH] Organize installer block device name blacklist. (#1148923)

Anne Mulhern amulhern at redhat.com
Wed Oct 8 12:48:07 UTC 2014





----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Wednesday, October 8, 2014 2:46:04 AM
> Subject: Re: [PATCH] Organize installer block device name blacklist. (#1148923)
> 
> On Tue, 2014-10-07 at 17:34 -0500, David Lehman wrote:
> > On 10/07/2014 04:54 PM, Anne Mulhern wrote:
> > >
> > >
> > >
> > >
> > > ----- Original Message -----
> > >> From: "David Lehman" <dlehman at redhat.com>
> > >> To: anaconda-patches at lists.fedorahosted.org
> > >> Sent: Tuesday, October 7, 2014 5:01:40 PM
> > >> Subject: [PATCH] Organize installer block device name blacklist.
> > >> (#1148923)
> > >>
> > >> ---
> > >>   blivet/devicetree.py | 4 ----
> > >>   blivet/udev.py       | 8 ++++++++
> > >>   2 files changed, 8 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> > >> index a5310b7..65379ee 100644
> > >> --- a/blivet/devicetree.py
> > >> +++ b/blivet/devicetree.py
> > >> @@ -638,10 +638,6 @@ class DeviceTree(object):
> > >>           if name.startswith("ram"):
> > >>               return True
> > >>
> > >> -        # Memory Technology Devices require special tools to
> > >> manipulate.
> > >> -        if name.startswith("mtd"):
> > >> -            return True
> > >> -
> > >>           if name.startswith("loop"):
> > >>               # ignore loop devices unless they're backed by a file
> > >>               return (not loop.get_backing_file(name))
> > >> diff --git a/blivet/udev.py b/blivet/udev.py
> > >> index c689b12..076fde2 100644
> > >> --- a/blivet/udev.py
> > >> +++ b/blivet/udev.py
> > >> @@ -26,6 +26,7 @@ import re
> > >>
> > >>   from . import util
> > >>   from .size import Size
> > >> +from .flags import flags
> > >>
> > >>   from . import pyudev
> > >>   global_udev = pyudev.Udev()
> > >> @@ -33,6 +34,9 @@ global_udev = pyudev.Udev()
> > >>   import logging
> > >>   log = logging.getLogger("blivet")
> > >>
> > >> +INSTALLER_BLACKLIST = ["^mtd", r'^mmcblk.+boot', r'^mmcblk.+rpmb',
> > >> "^zram"]
> > >
> > > Could you
> > >
> > > 1) make this a tuple, it doesn't need all of list capability
> > 
> > Sure. I started with a tuple and changed it to a list on a whim.
> > 
> > > 2) use raw strings for all expressions, it's consistent and can do no
> > > harm
> > 
> > I suppose I could do that.
> > 
> > > 3) make this local to enumerate_devices() (unless you have a particular
> > > reason for making it file global, of course)
> > 
> > I thought it would be easier for people reading the code and/or looking
> > for this functionality. Also, it's a constant, so I knew someone would
> > ask me to upper-case it, but then that looks weird for a function local,
> > so here we are.
> Agreed, I think it's better visible as a global constant. Since it has a
> mnemonic name it's easy to update by somebody who has now idea there is
> some enumerate_devices() function.
> 

That's exactly what I'm afraid of.

Please make it method local until there's a good reason to make it file local
instead.

> > 
> > >
> > > - mulhenr
> > >
> > >> +""" device name regexes to ignore when flags.installer_mode is True """
> > >> +
> > >>   def enumerate_devices(deviceClass="block"):
> > >>       devices = global_udev.enumerate_devices(subsystem=deviceClass)
> > >>       return [path[4:] for path in devices]
> > >> @@ -171,6 +175,10 @@ def __is_blacklisted_blockdev(dev_name):
> > >>       if dev_name.startswith("ram") or dev_name.startswith("fd"):
> > >>           return True
> > >>
> > >> +    if flags.installer_mode:
> > >> +        if any(re.search(expr, dev_name) for expr in
> > >> INSTALLER_BLACKLIST):
> > >
> > > Could you just use match() instead of search() and get rid of the '^''s
> > > in regular expressions?
> > 
> > I could, but I thought it was easier to understand this way. Also, more
> > flexible. I considered doing it, but it felt like a shortcut.
> Both ways look understandable to me, but this really is more flexible so
> I'd leave it as it is.
> 

Yeah, it is more flexible.

> --
> Vratislav Podzimek
> 
> Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 


More information about the anaconda-patches mailing list