[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] Added the libudev python bindings



> Also rewrote the baseudev.py to use the bindings instead of parsing
> the udev db file
> ---
>  baseudev.py |   66 ++++++----------------
>  pyudev.py   |  180 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 197 insertions(+), 49 deletions(-)

Will pyudev.py eventually be moving into another package, like perhaps
in the udev package or one of its subpackages?

> @@ -43,20 +44,24 @@ def udev_get_device(sysfs_path):
>          log.debug("%s does not exist" % sysfs_path)
>          return None
>  
> -    db_entry = sysfs_path.replace("/", "\\x2f")
> -    db_root = "/dev/.udev/db"
> -    db_path = os.path.normpath("%s/%s" % (db_root, db_entry))
> -    if not os.access(db_path, os.R_OK):
> -        log.debug("db entry %s does not exist" % db_path)
> -        return None
> +    udev = pyudev.Udev()
> +    # XXX we remove the /sys part when enumerating devices,
> +    # so we have to prepend it when creating the device
> +    udev_device = udev.create_device("/sys" + sysfs_path)
> +    udev.teardown()
> +
> +    dev = udev_device.properties
> +
> +    if dev:
> +        # remove the /dev/ part from beginning of devnode and set as name
> +        dev["name"] = udev_device.devnode[5:]
> +
> +        dev["symlinks"] = dev["DEVLINKS"].split()
> +        dev["sysfs_path"] = sysfs_path
>  
> -    entry = open(db_path).read()
> -    dev = udev_parse_entry(entry)
> -    if dev.has_key("name"):
> -        dev['sysfs_path'] = sysfs_path
> +        # now add in the contents of the uevent file since they're handy
>          dev = udev_parse_uevent_file(dev)
>  
> -    # now add in the contents of the uevent file since they're handy
>      return dev
>  
>  def udev_get_devices(deviceClass="block"):

We could potentially be calling udev_get_device() on several hundreds or
even thousands of devices, which is a whole lot of:

   pyudev.Udev()
   udev.create_device()
   udev.teardown()

Can we instead only call pyudev.Udev() once and stash an instance to the
object as a global in baseudev.py?  I'm concerned that we're going to be
introducing another big slowdown here in device discovery on very large
configurations.

> +# XXX this one may need some tweaking...
> +def find_library(name):
> +    libdirs = ("/lib64", "/lib")
> +
> +    for dir in libdirs:
> +        files = fnmatch.filter(os.listdir(dir), "*%s*" % name)
> +        files = [os.path.join(dir, file) for file in files]
> +
> +        if files:
> +            break
> +
> +    if files:
> +        return files[0]
> +    else:
> +        return None

Would be nice to search the $LD_LIBRARY_PATH, since we use that to do
updates.img stuff:

   def find_library(name):
      env = os.environ.get("LD_LIBRARY_PATH")
      if env:
         libdirs = env.split(":")
      else:
         libdirs = ["/lib64", "/lib"]

      for dir in libdirs:
      ...

> +class UdevDevice(object):

If you really want to be fancy, you can add some special methods to
UdevDevice so people don't have to go through the properties dict to
access udev properties.  You could do something like the following:

   def __getattr__(self, name):
      return self.properties[name]

Then it's just a simple matter of calling it like this:

   udev_device = udev.create_device("/sys" + sysfs_path)
   print udev_device["symlinks"]
   print udev_device["busted"]

The first print will return udev_device.properties["symlinks"].  The
second will raise AttributeError, like you'd expect.  I think doing
something like this makes for a much nicer API and hides some of the
implementation details.

More information at:

   http://docs.python.org/reference/datamodel.html?highlight=__get__#customizing-attribute-access

> +class Udev(object):
> +
> +    def __init__(self):
> +        self.udev = libudev_udev_new()
> +
> +    def create_device(self, sysfs_path):
> +        return UdevDevice(self.udev, sysfs_path)
> +
> +    def scan_devices(self, subsystem=None):
> +        enumerate = libudev_udev_enumerate_new(self.udev)

scan_devices could take a while, I suppose.  Is it worth making it
return an iterator so it doesn't have to grab everything all at once and
can instead grab each individual device as needed?  Or am I just making
things too complicated now?

> +        # add the match subsystem
> +        if subsystem is not None:
> +            rc = libudev_udev_enumerate_add_match_subsystem(enumerate, subsystem)
> +            if not rc == 0:
> +                print >> sys.stderr, "error: unable to add the match subsystem"
> +                libudev_udev_enumerate_unref(enumerate)
> +                return []

print is a function in python3 (also python2.6?).  For future
compatibility, you'll want to:

   print("error: whatever", file=sys.stderr)

One final minor point:  This is a fairly low level module in anaconda.
Whenever we add new things like this, it's good to think about how it
could be automatically tested so we have more trust in what gets built
on top of it.  Have you given any thought to this yet?'

Overall, looks interesting.

- Chris


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]