Hi Zhongyuan.
Thanks for your feedback.
On 12/16/09 22:45, zhongyuan sun - Sun Microsystems - Beijing China wrote:
> Hi Jack,
>
> Thanks your quick response. my comments:
>
> (1)I'm not very clear about class devscan_data, what its main function?
> so far I think it's no more different with a list type. The main
> scanning function was implemented by ddu_devscan(). If devscan_data only
> provide like append function, I suggest just use a list but not this
> additional class.
>
The devscan_data class bundles the data with search functionality. (I
assume this search functionality is something you need, since you had
originally suggested using a dictionary.) get_list_by_devtype(devtype)
is the search method.
A list by itself could be used, but then you would need to add external
search methods to filter the data, (like all devices of type "Audio").
It is convenient to put those methods into the same class as the data.
Why my class instead of a dictionary? Because it is simpler and more
flexible than a dictionary. This class is flexible as you can add
additional search methods in the future if needed.) If a dictionary is
used instead then this additional flexibility becomes harder to
implement. I would think (but haven't verified this) that dictionaries
are more heavyweight than the dev_data class.
> (2)Which class will in charge of store "Device compatible name" string
> used by ddu_package_lookup()? I did not put it in class dev_data because
> I think this class only store general device information, while "Device
> compatible name" is not a general information especially for that device
> which already has a working driver. "Device compatible name" will be
> only useful for fetching driver information. Shall we add a new function
> to generate "Device compatible name"? This function's argument can be
> pci path and output is "Device compatible name".
>
From what I see, dev_data stores data specific to a device. (For
example, attach_status and instance_ID are very specific to a device.)
The compatible name string is another piece of device specific data, and
so I think we should include it in dev_data. (Note: it is currently
missing from dev_data; that is an oversight and will be corrected.)
If the compatible name string is in dev_data, do you still need the
vendor_id, device_id and class_code? (I think you do, as you can use
these items to locate a specific kind of device if necessary, but please
confirm.)
You can already retrieve a device's compatible name string from the
kernel via prtconf (and probably other ways too). Since the device
compatible name string is already generated elsewhere, let's not create
another function to generate it again. Duplication of existing
functionality makes the code more difficult to maintain.
> (3)function ddu_devscan() and ddu_package_lookup() will take a little
> long time when it running, also during this time these two functions
> will fork some child processes. Should we provide a mechanism to stop
> these functions during it running then they can kill all their child
> processes? This situation usually happens in GUI environment.
>
I think providing a way for the parent to kill its child processes is a
good idea. However, I would try to make it automatic via a signal
handler. When the parent receives a signal to die, have it catch that
catch and process its signal by signaling each of its children to die
gracefully before terminating itself.
Thanks,
Jack
> Also thanks Keith's review, I agree with his comments.
>
> Thank,
> Zhongyuan
>
> ?2009?12?17??07:02?Jack Schwartz???
>
>> Hi Zhongyuan.
>>
>> Thanks for your feedback. Here's my assessment of requirements, based
>> on both of our emails, and a new proposal which I believe addresses
>> both
>> of our needs and concerns. Hopefully it is not too long...
>>
>> A) You need a way to easily restrict the output to a given device
>> type.
>> I assume this why you proposed a dictionary keyed to the device type,
>> at the top level. I didn't account for that in my original proposal.
>>
>> B) My proposal placed more of an emphasis on readability and clarity.
>> This is why I originally proposed to store a device's attributes in a
>> dictionary: attributes could be extracted by name.
>>
>> Additional requirements, as I see things:
>>
>> C) We don't want to make the data structures too complicated from a
>> data
>> access perspective. For example a dictionary of lists of dictionaries
>> makes the code hard to follow.
>>
>> D) We don't want to introduce unnecessary overhead. If we can use
>> something more lightweight than a dictionary and still have the
>> functionality we need, then I would favor not using the dictionary.
>>
>>
>> ddu_devscan() will be returning a group of items corresponding to
>> devices, so it is appropriate to use a collection of some kind for the
>> top level data structure it returns. At first I agreed with you that
>> a
>> dictionary would be best at the top level:
>> - its items are named (via a key),
>> - it can filter out unwanted items,
>> - it doesn't have to keep a place for items it doesn't need.
>> However, after thinking more, this could prove restrictive in the
>> future. We return lots of information for each device, any of which
>> could potentially be used as a filter for returning data. If the
>> dictionary is keyed on device type and later we decide we need to
>> filter on something else (or perhaps multiple items), say vendor ID
>> and/or device ID, we're not set up for that.
>>
>> Instead, I propose a new class, call it devscan_data, which stores the
>> full list of items returned by ddu_devscan() and has a method for
>> searching and returning only items of a particular device type. Then
>> in the future we could add other methods to support returning items
>> based on some other filter criteria. It is also lightweight and
>> provides only the functionality needed.
>>
>> What are the devscan_data list items? They would store the different
>> fields of data per device. The list could point to dictionaries as I
>> had originally proposed, to tuples as you have proposed, or to
>> something
>> else. I have changed my mind about dictionaries, and think they are
>> the
>> wrong tool; the number and type of fields will not change from one
>> device to the next. I also still don't like tuples as the fields are
>> hard to keep track of. Named tuples are better, but more heavyweight
>> than if we defined a new class to hold the data. So I propose a new
>> class, which would have attributes and get() methods for each field we
>> want to return. Let's call it dev_data for now.
>>
>> To summarize:
>>
>> ddu_devscan() would return a devscan_data object defined like this:
>>
>> class devscan_data():
>>
>> def __init__()
>> # Holds objects of dev_data.
>> self.dev_data_list = []
>>
>> def add_dev_data_object(dev_data):
>> self.dev_data_list.append(dev_data)
>>
>> def get_list_by_devtype(device_type):
>> # loop through instance list data to find and return matches
>> on
>> # device_type
>>
>> Lists returned by get_list_by_devtype() point to objects of type
>> dev_data.
>> There is one of these objects per device returned.
>>
>> class dev_data:
>>
>> def __init__(...):
>> self.device_type = ...
>> self.description = ...
>> self.driver_name
>> self.instance_ID
>> self.item_ID
>> self.parent_item_ID
>> self.attach_status
>> self.vendor_id
>> self.device_id
>> self.class_code
>> self.pci_path
>>
>> def get_device_type():
>> return self.device_type
>>
>> def get_description():
>> return self.description
>>
>> ...
>> ...
>>
>> def get_pci_path:
>> return self.pci_path
>>
>> Does this work for you?
>>
>> Thanks,
>> Jack
>>
>> On 12/15/09 21:56, zhongyuan sun - Sun Microsystems - Beijing China
>> wrote:
>>
>>> Hi Jack,
>>>
>>> It's OK ddu_devscan return dictionary type instead of a list of tuples.
>>> I suggest use device type as this dictionary's key and the dictionary's
>>> value would be a list of tuples.
>>>
>>> ddu_devscan description would be as following:
>>>
>>> Arguments:
>>> - return_missing_only: Boolean; default value is True
>>> - When True, ddu_devscan returns only the devices which are missing
>>> drivers.
>>> - When False, all devices found are returned.
>>> - device_type: Type of devices to scan for. "All" to check all.
>>> - Possible types
>>> are:"audio"/"battery"/"cpu"/"cd_dvd"/"memory"/"network"/"storage"/"usb"/"video"/"others"
>>>
>>>
>>> In each tuple of the list:
>>> - ItemID:
>>> - Parent Item ID:
>>> /*these two fields(ItemID and Parent Item ID) used for device tree to
>>> identify a device and the controller it attached to. Of course
>>> for a
>>> controller we do not need to set its parent Item ID.
>>> */
>>> - Device description:Device name
>>> - Device ID:
>>> - Classcode:
>>> - PCI physical path:This field used to get its detail information.
>>> - Driver name:
>>> - Instance ID:
>>> - Attachment status:
>>> /*These two fields(Instance ID and Attachment status) used for
>>> determine whether a driver
>>> has been attached abnormally.
>>> */
>>> - Vendor ID
>>>
>>>
>>> Here is a short example of ddu_devscan return value:
>>>
>>>
>>>>>> {'Audio': [(200,"","nVidia Corporation MCP55 High Definition
>>>>>>
>>>>>>
>>> Audio","0x0371","00040300","[0,6,1]","unknown",-1,"Detached","0x10de")],'Network':[(0,"","nVidia
>>> Corporation MCP55
>>> Ethernet","0x0373","00068000","[80,8,0]","nge",0,"Attached","0x10de"),(1,"","nVidia
>>> Corporation MCP55
>>> Ethernet","0x0373","00068000","[80,9,0]","nge",1,"Attached","0x10de")],'DVD':[(400,"","nVidia
>>> Corporation MCP55
>>> IDE","0x036e","0001018a","[0,4,0]","pci-ide",0,"Attached","0x10de"),(401,400,"MATSHITA
>>> DVD-RAM UJ-85JS","","","/pci at 0,0/pci-ide at 4/ide at 0/sd at
>>> 0,0","sd",0,"Attached","")]}
>>>
>>>
>>> I also would like to discuss with you a issue during scan process. When
>>> user kill GUI during scan process, some backround scripts will still be
>>> running even GUI thread has been killed, these scripts was created when
>>> performing scanning. Our solution was send a SIGUSR2 signal when GUI
>>> want to kill its child processes. While if we wrapped our scripts into
>>> ddu_devscan, how can this function send a signal to its child processes?
>>> can we add a argument for this?
>>>
>>>
>>> Thanks,
>>> Zhongyuan
>>>
>>> ?2009?12?15??06:43?Jack Schwartz???
>>>
>>>
>>>> Hi Zhongyuan.
>>>>
>>>> Thanks for reviewing the interface spec. Moving to public forum.
>>>>
>>>> > I've some comments on DDU library API--ddu_devscan() function.
>>>> > You've expanded this function to involve all devices found, it's
>>>> > good! While for the Return value, currently it only includes
>>>> > "compatible name" and "device descriptor" strings, It's not
>>>> > sufficient especially for GUI part:
>>>> >
>>>> > *For non-mission items
>>>> > (1)If the driver works fine,we need a "driver name" field.
>>>> >
>>>> OK
>>>>
>>>> > (2)If the driver dos not works fine, wee need the driver instance
>>>> > ID and attachment status for this driver.
>>>> >
>>>> > Instance ID is a number get from system when driver attached. If
>>>> > you have 2 e1000g nic cards and they all have been attached
>>>> > properly, then they will get instance 0 and 1, while if the driver
>>>> > have not been attached properly or device have been installed
>>>> > properly, the instance ID will be <0 and attachment status will be
>>>> > "detached".
>>>> >
>>>> > DDU GUI version used "instance ID" and attachment status to give
>>>> > user a notification "driver missconfigured".
>>>>
>>>> OK
>>>>
>>>> >
>>>> > BTW, for both non-missing and missing device, I hope we can add
>>>> > "device category" argument to ddu_devscan function to indicate what
>>>> > type of category will be looked for(like "Audio"/"Network" etc). By
>>>> > default this argument can be set to "All" to look for all types of >
>>>> device.
>>>>
>>>> OK.
>>>>
>>>> > For both missing and non-missing items, we also need these fields
>>>> > at least:
>>>> > (1)"Vendor id"/"device id"/"classcode" to identify which category
>>>> > this device belongs to.
>>>>
>>>> OK.
>>>>
>>>> > (2)pci path to indicate this device's detail information.
>>>> > Hopefully this can be involved into ddu_devscan function.
>>>>
>>>> OK.
>>>>
>>>> These are a lot of changes for ddu_devscan() for the GUI. I propose
>>>> that ddu_devscan returns a list of dictionaries rather than a list of
>>>> tuples (as before). Dictionaries because there are a lot of items
>>>> returned. Also, not all fields will be used for all devices. (For
>>>> example, the driver_name field won't be used when a device is missing).
>>>>
>>>> So this would translate into adding a signature of ddu_devscan() as
>>>> follows:
>>>>
>>>> Arguments:
>>>> - return_missing_only: Boolean; default value is True
>>>> - When True, ddu_devscan returns only the devices which are missing
>>>> drivers.
>>>> - When False, all devices found are returned.
>>>> - device_type: Type of devices to scan for. "All" to check all.
>>>>
>>>> ===> Please send the list of device types which are valid. "Audio" and
>>>> "Network" were mentioned earlier.
>>>>
>>>> Returns:
>>>> A list of dictionaries, each dictionary representing a device. List can
>>>> be empty (possible only when Return_missing_only is True) to represent a
>>>> system with no missing drivers.
>>>>
>>>> Items in each dictionary are:
>>>>
>>>> - compatible: device compatible name string, a space-separated string of
>>>> all compatible names for a device. Compatible names in the string are
>>>> ordered from the most specific name to the most general name (i.e. the
>>>> "pciclass" definitions are listed last).
>>>> - It is OK if two list items have one or more compatible
>>>> names in common, but each list item should be unique.
>>>>
>>>> - description: Device description. can be used for error messages and
>>>> status reporting.
>>>>
>>>> - device_type: Type of device (e.g. "Audio", "Network")
>>>>
>>>> - driver_name: Name of the driver operating the device. (Can be omitted
>>>> if driver is missing.)
>>>>
>>>> - instance_ID: Instance ID assigned to the device. (Always present.
>>>> Will be <0 if no driver is attached to the device.)
>>>>
>>>> - attach_status: Status of whether or not a driver is attached to the
>>>> device. (Always present. Will be "detached" if driver is not attached,
>>>> "attached" if driver is attached.
>>>>
>>>> ===> Any other statuses possible?
>>>>
>>>> - vendor_id: Device's vendor ID. (Always present.)
>>>>
>>>> - device_id: Device's device ID (Always present.)
>>>>
>>>> - class_code: Device's class code (Always present.)
>>>>
>>>> - pci_path: Device path (Always present.)
>>>>
>>>> Please let me know by today COB (PRC time) the device types and if any
>>>> corrections are needed.
>>>>
>>>> Thanks,
>>>> Jack
>>>>
>>>> On 12/04/09 09:20, Jack Schwartz wrote:
>>>>
>>>>
>>>>> Hi everyone.
>>>>>
>>>>> FYI: Driver Update functional and programming interface specs have had
>>>>> a minor update. The items addressed deal either with subtleties
>>>>> missed during the main review but which came up during development, or
>>>>> other things we discovered were needed during development.
>>>>>
>>>>> Functional spec:
>>>>> - Clarify that any repo can be specified, not only repos which a
>>>>> system already
>>>>> knows about.
>>>>> - Clarify that it is an error if a publisher name is already mapped to
>>>>> a repo,
>>>>> and another repo with the same publisher name is requested.
>>>>> - Add publisher to AI searchall capability
>>>>>
>>>>> Interface spec:
>>>>> - Add arg to ddu_devscan() so that the function can return either all
>>>>> devices
>>>>> on a system, or only devices which are missing their driver.
>>>>>
>>>>> Links on the Driver Update page have been updated. Previous spec
>>>>> versions have been retained on that page for comparison purposes.
>>>>>
>>>>> Driver update page is at:
>>>>> http://hub.opensolaris.org/bin/view/Project+caiman/Driver_Update
>>>>>
>>>>> If there are any questions or comments please send them my way.
>>>>>
>>>>> Thanks,
>>>>> Jack
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20091217/ac6acc61/attachment.html>
