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