Hello,
please see my answers below.
--
Martin Gracik
----- "Chris Lumens" <clumens@xxxxxxxxxx> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
