[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