Hi Zhongyuan.
On 12/17/09 22:20, zhongyuan sun - Sun Microsystems - Beijing China wrote:
> Looks good! a little clarification,
>
> ?2009?12?18??08:17?Jack Schwartz???
>
>> 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.
>>
>
> ddu_devscan function will be used in GUI/AI/Text-mode parts.
> While in GUI part, ddu_devscan will be called separately for each
> components(audio,network,storage....), the source code would be
> like:ddu_devcan(return_missing_only=All, device_type="audio") and then
> ddu_devcan(return_missing_only=All, device_type="network") and on. In
> this way, ddu_devscan will do the search method, its return value(will
> be stored in class devscan_data) do not need to search again.
> For AI/Text-mode parts, ddu_devscan will be called only once and return
> all missing driver components. But since AI/Text-mode do not care about
> which type(audio,network...) the devices belongs to, so in this way even
> class devscan_data provide get_list_by_devtype method, it will never be
> called.
> In a summary, I do not know when this method get_list_by_devtype in
> class devscan_data will be called.
>
Thanks for your clarification. So get_list_by_devtype() is not needed,
and we can just use a list at the top level.
ddu_devscan() will now return a list of dev_data objects.
>
>> 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.)
>>
>>
> Yes, compatible name string could be a property of class dev_data. While
> from the implementation point of view, all the current dev_data 11
> properties(description,driver_name,instance_ID...) are get from one
> session(they are output from a DDU tool). We do not include compatible
> name string as a default output because its only useful for missing
> driver components. For that non-missing driver component DDU does not
> use it. So for most devices the compatible name string is not a very
> critical property for DDU, DDU only fetch it when needed that is a
> missing driver device want to look for its driver.
> Of course DDU can get compatible name string very easy, but it need some
> more codes in another block. That why i suggest to create another method
> in dev_data.
> Again the dev_data 11
> properties(description,driver_name,instance_ID...)are get from one
> session and compatible name string will be fetched from another session
> even if the method is very easy.
> If compatible name string is a property of class dev_data, From
> performance point of view, the non-missing driver device will set it to
> None and missing driver device will set to its real value?
>
A specification assumes (and therefore limits) the use of a field when
it specifies that field is to be set only under certain conditions.
This is not always a bad thing, but a more versatile solution is to
always set the field if it is going to be present. So I suggest to
either always set the compatible name or else to not include it in the
structure.
Let's take the suggestion you made yesterday, and have a separate
function to return the compatible name string, when given a dev_data
object argument. This way, the DDU can get the compatible name when it
needs it (and so performance around this will be optimized), and
dev_data fields will be consistent.
How about this:
def get_compatible_name(dev_data):
# Returns compatible name string
>> 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.)
>>
> Yes
>
OK.
>
>> 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.
>>
> This is exactly what i want, all the child process will be killed via
> signal. I just want a way for these two functions ddu_devscan() and
> ddu_package_lookup() can send kill signals to their child processes. For
> example, can we set a argument for these functions? If the function
> found this argument has been set to True, then they can have a chance to
> send kill signals to child processes.
>
I think what you are suggesting is that ddu_devscan and
ddu_package_lookup will spawn subprocesses when called with an argument
"termination=False" and to kill the subprocesses they spawned when
called with "termination=True". Do I understand correctly?
Assuming I understand correctly, I don't think the additional arg is
necessary. Just have their parent (whoever calls ddu_devcscan and
ddu_package_lookup) send a signal to the process group at termination
time. This will terminate those subprocesses plus all of their
children. If you don't want to terminate the parent at that time, set
it up to ignore the signal before sending it.
I wrote and attached a sample shell and python program to show how this
can be done. You may need to do a hybrid approach of the two programs
depending on how your library is set up.
Since we seem to be converging, I'm going to update the spec again. If
you have more issues or if I didn't understand you correctly, please
reply ASAP.
Thanks,
Jack
P.S. Don't use these sample programs as examples of final form. Though
they are reasonably clean, I haven't checked them for PEP8 compliance or
style conformity, etc.
> Thanks,
> Zhongyuan
>
>> 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/20091218/02c6bf3e/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: childterm.ksh
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20091218/02c6bf3e/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: childterm.py
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20091218/02c6bf3e/attachment-0001.ksh>
