Hi,
please check my answers below your suggestions.
--
Martin Gracik
----- Original Message -----
From: "Hans de Goede" <hdegoede@xxxxxxxxxx>
To: anaconda-devel-list@xxxxxxxxxx
Sent: Tuesday, November 10, 2009 5:24:29 PM GMT +01:00 Amsterdam / Berlin / Bern / Rome / Stockholm / Vienna
Subject: Re: [PATCH] Added the libudev python bindings
Hi,
On 11/10/2009 04:19 PM, Martin Gracik 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(-)
> create mode 100644 pyudev.py
>
> diff --git a/baseudev.py b/baseudev.py
> index 76b14cc..978bae0 100644
> --- a/baseudev.py
> +++ b/baseudev.py
> @@ -23,6 +23,7 @@
>
> import iutil
> import os
> +import pyudev
>
> import logging
> log = logging.getLogger("storage")
> @@ -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()
> +
Not sure if I like the teardown() name, this implies that after this
things can be setup again, which is not true.
Ok, I have no problem with changing the name, suggest one for me that would be appropriate.
> + dev = udev_device.properties
> +
> + if dev:
> + # remove the /dev/ part from beginning of devnode and set as name
> + dev["name"] = udev_device.devnode[5:]
> +
This is not correct, this will return mapper/Volgroup-Volname for LV's where it
should return just Volgroup-Volname, please find out which libudev function return
the actual internal libudev name and use that instead.
It is correct if we want to have the same stuff the old baseudev.py returns.
I was using just the SYSNAME, but that's dm-0, dm-1 etc. But the old baseudev.py returns mapper/vg-something,
so I checked all the properties, and libudev functions, and the only one getting this kind of output was
devnode(), but with the /dev/ at the start.
> + 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"):
> @@ -68,45 +73,8 @@ def udev_get_devices(deviceClass="block"):
> entries.append(entry)
> return entries
>
> -def udev_parse_entry(buf):
> - dev = {}
> -
> - for line in buf.splitlines():
> - line.strip()
> - (tag, sep, val) = line.partition(":")
> - if not sep:
> - continue
> -
> - if tag == "N":
> - dev['name'] = val
> - elif tag == "S":
> - if dev.has_key('symlinks'):
> - dev['symlinks'].append(val)
> - else:
> - dev['symlinks'] = [val]
> - elif tag == "E":
> - if val.count("=")> 1 and val.count(" ")> 0:
> - # eg: LVM2_LV_NAME when querying the VG for its LVs
> - vars = val.split()
> - vals = []
> - var_name = None
> - for (index, subval) in enumerate(vars):
> - (var_name, sep, var_val) = subval.partition("=")
> - if sep:
> - vals.append(var_val)
> -
> - dev[var_name] = vals
> - else:
> - (var_name, sep, var_val) = val.partition("=")
> - if not sep:
> - continue
> -
> - dev[var_name] = var_val
> -
> - return dev
> -
> def udev_parse_uevent_file(dev):
> - path = os.path.normpath("/sys/%s/uevent" % dev['sysfs_path'])
> + path = os.path.normpath("/sys/%s/uevent" % dev["sysfs_path"])
This is a meaningless change, which does not belong in this patch
Ok, I know it's meaningless, but when I was editing the file, I wanted to have the code consistent.
Didn't know that's a problem. Will change it back.
> if not os.access(path, os.R_OK):
> return dev
>
> diff --git a/pyudev.py b/pyudev.py
> new file mode 100644
> index 0000000..8d5a7c2
> --- /dev/null
> +++ b/pyudev.py
> @@ -0,0 +1,180 @@
> +import sys
> +import os
> +import fnmatch
> +from ctypes import *
> +
> +
> +# 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
> +
Please make the matching here less flexible, we want something
mathching "lib%s.so.%d" % (name, somajor), where somajor is a
new parameter to find_library, this way we can also detect if the ABI
ever changes underneed us, as then the somajor will change and we
will no longer find the library.
I was concerned myself about this function, wasn't sure what all kind of names
the library can have, so I just put some default matching expression.
Will change to what you proposed.
> +
> +# find the udev library
> +libudev = find_library("udev")
> +
> +if not libudev or not os.path.exists(libudev):
> + raise ImportError, "No library named %s" % libudev
> +
> +# load the udev library
> +libudev = CDLL(libudev)
> +
> +
> +# create aliases for needed functions and set the return types where needed
> +libudev_udev_new = libudev.udev_new
> +libudev_udev_unref = libudev.udev_unref
> +
> +libudev_udev_device_new_from_syspath = libudev.udev_device_new_from_syspath
> +libudev_udev_device_unref = libudev.udev_device_unref
> +
> +libudev_udev_device_get_syspath = libudev.udev_device_get_syspath
> +libudev_udev_device_get_sysname = libudev.udev_device_get_sysname
> +libudev_udev_device_get_syspath.restype = c_char_p
> +libudev_udev_device_get_sysname.restype = c_char_p
> +
> +libudev_udev_device_get_properties_list_entry = libudev.udev_device_get_properties_list_entry
> +libudev_udev_list_entry_get_next = libudev.udev_list_entry_get_next
> +
> +libudev_udev_list_entry_get_name = libudev.udev_list_entry_get_name
> +libudev_udev_list_entry_get_value = libudev.udev_list_entry_get_value
> +libudev_udev_list_entry_get_name.restype = c_char_p
> +libudev_udev_list_entry_get_value.restype = c_char_p
> +
> +libudev_udev_enumerate_new = libudev.udev_enumerate_new
> +libudev_udev_enumerate_unref = libudev.udev_enumerate_unref
> +
> +libudev_udev_enumerate_add_match_subsystem = libudev.udev_enumerate_add_match_subsystem
> +libudev_udev_enumerate_scan_devices = libudev.udev_enumerate_scan_devices
> +libudev_udev_enumerate_get_list_entry = libudev.udev_enumerate_get_list_entry
> +
> +
> +class UdevDevice(object):
> +
> + def __init__(self, udev, sysfs_path):
> + self.syspath = None
> + self.sysname = None
> + self.properties = {}
> +
> + # create new udev device from syspath
> + udev_device = libudev_udev_device_new_from_syspath(udev, sysfs_path)
> + if not udev_device:
> + # device does not exist
> + return
> +
> + # set syspath and sysname properties
> + self.syspath = libudev_udev_device_get_syspath(udev_device)
> + self.sysname = libudev_udev_device_get_sysname(udev_device)
> +
> + # get the first property entry
> + property_entry = libudev_udev_device_get_properties_list_entry(udev_device)
> +
> + while property_entry:
> + name = libudev_udev_list_entry_get_name(property_entry)
> + value = libudev_udev_list_entry_get_value(property_entry)
> +
> + # XXX we have to split some of the values into a list,
> + # the libudev is not so smart :(
> + fields = value.split()
> +
> + if len(fields)> 1:
> + value = [fields[0]]
> +
> + for item in fields[1:]:
> + (key, sep, val) = item.partition("=")
> + if sep:
> + value.append(val)
> +
> + if len(value) == 1:
> + value = value[0]
> +
> + self.properties[name] = value
> +
> + # get next property entry
> + property_entry = libudev_udev_list_entry_get_next(property_entry)
> +
> + # set additional properties
> + libudev.udev_device_get_devpath.restype = c_char_p
> + self.devpath = libudev.udev_device_get_devpath(udev_device)
> +
> + libudev.udev_device_get_subsystem.restype = c_char_p
> + self.subsystem = libudev.udev_device_get_subsystem(udev_device)
> +
> + libudev.udev_device_get_devtype.restype = c_char_p
> + self.devtype = libudev.udev_device_get_devtype(udev_device)
> +
> + libudev.udev_device_get_sysnum.restype = c_char_p
> + self.sysnum = libudev.udev_device_get_sysnum(udev_device)
> +
> + libudev.udev_device_get_devnode.restype = c_char_p
> + self.devnode = libudev.udev_device_get_devnode(udev_device)
> +
> + # cleanup
> + libudev_udev_device_unref(udev_device)
> +
> + def __str__(self):
> + s = "SYSPATH: %s\n" % self.syspath
> + s += "SYSNAME: %s" % self.sysname
> +
> + return s
> +
> +
> +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)
> +
> + # 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 []
> +
> + # scan the devices
> + rc = libudev_udev_enumerate_scan_devices(enumerate)
> + if not rc == 0:
> + print>> sys.stderr, "error: unable to scan the devices"
> + libudev_udev_enumerate_unref(enumerate)
> + return []
> +
> + # create the list of devices
> + devices = []
> +
> + # get the first device entry
> + device_entry = libudev_udev_enumerate_get_list_entry(enumerate)
> +
> + while device_entry:
> + sysfs_path = libudev_udev_list_entry_get_name(device_entry)
> + device = self.create_device(sysfs_path)
> +
> + if device.syspath is not None:
> + devices.append(device)
> +
> + # get next device entry
> + device_entry = libudev_udev_list_entry_get_next(device_entry)
> +
> + # cleanup
> + libudev_udev_enumerate_unref(enumerate)
> +
> + return devices
> +
> + def teardown(self):
> + libudev_udev_unref(self.udev)
You will want to set self.udev to None after this, and check all other uses for it to be not None, other
wise you may cause a segfault (referring freed memory) !
Will do.
Thanks for the review.
Regards,
Hana
_______________________________________________
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
