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

Re: [PATCH] Added the libudev python bindings



Hello,

please see my answers below.

--

  Martin Gracik

----- "Chris Lumens" <clumens redhat com> wrote:

> > 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?

Well, I cannot answer this now, because I don't know if we want
this to happen, but I think it is possible. Later maybe I would like
to add code for udevadm settle and trigger, but as I was looking at 
the C code for that, it's not so trivial, so I didn't want to break
anything for RHEL6. Maybe I can do it later, for F13, and when it
will have more features, we can move it to some other package.
Didn't want to make a lot of different changes to break some things now.

> 
> > @@ -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.

Propably you're right, I have no idea how many times this get's called.
What I wanted to do first was rewrite the whole baseudev.py to something "better".
But Hans stopped me, to rewrite just needed parts, not to break anything.
So I was just trying to make as little changes as possible to the original code.

My concern is, where will we call the "teardown" method?

> 
> > +# 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:
>       ...

Will add this to the function, along with the changes Hans proposed.

> 
> > +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

I know about this python feature. But when using this, I would have to return the whole UdevDevice object,
from the udev_get_devices().
Because the old functions return just a dict, with properties. Will this be OK ?

> 
> > +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?

Don't have much experience with iterators. I will take a look at that.

> 
> > +        # 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)

OK, I will change the print stuff.

> 
> 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?'

OK, I will try to come up with something. But I don't promise anything yet :)

> 
> Overall, looks interesting.
> 
> - Chris

Thank you for the comments.

> 
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list redhat com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list


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