Hi Jack,
Thanks your feedback, a couple of commets below.
Thanks,
Tony
Jack Schwartz wrote:
> Hi Tony.
>
> Thanks for making the changes.
>
> A couple of comments below.
>
> On 03/ 8/10 01:06 AM, Tony Hu wrote:
>> Hi Jack,
>> Thanks your feedback, I had updated the source code and
>> webrev(http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev_20100308/),
>> see my comments below.
>>
>> Thanks,
>> Tony
>>
>> Bill Yan wrote:
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>>
>>> Subject:
>>> Re: [caiman-discuss] Please review Device Driver Utility(DDU)
>>> changes for Driver Update
>>> From:
>>> Jack Schwartz <Jack.A.Schwartz at Sun.COM>
>>> Date:
>>> Thu, 04 Mar 2010 23:59:01 -0800
>>> To:
>>> Bill Yan <Xue-Yang.Yan at Sun.COM>
>>>
>>> To:
>>> Bill Yan <Xue-Yang.Yan at Sun.COM>
>>> CC:
>>> caiman-discuss <caiman-discuss at opensolaris.org>
>>>
>>>
>>>
>>> Hi Bill and DDU Team.
>>>
>>> In general, most of the files look really good now! Thanks for
>>> adding comments to most files, and for creating the libddudev.c
>>> library of common functions. This makes the code much more
>>> maintainable than before.
>>>
>>> Changes to a few files near the bottom of the list were not done.
>>>
>>> In this pass I have overlooked the less-important things. I have
>>> included comment requests if they are easy, and code requests which
>>> I made before and were not done. (Fortunately these changes are easy
>>> and straightforward.) Please fix these or else state why you don't
>>> want to change them.
>>>
>>> Please do this and get changed files back to me ASAP to review so we
>>> can complete this review.
>>>
>>> Below are my specific comments:
>>>
>>> all_devices/Makefile:
>>> - Thanks for making a library ddudev.
>>> - Please remove the $(DBX) reference on line 27:
>>> CFLAGS +=
>>>
>>> all_devices/all_devices.c:
>>> 115: typo Ouput -> Output
>>>
>>> all_devices/all_devices.h: OK
>>>
>>> ids.c:
>>> 588: "must great 0" -> "must be > 0"
>>>
>>> ids.h: OK
>>>
>>> bat_detect/Makefile:
>>> 27: CFLAGS +=
>>>
>>> bat_detect.c:
>>> 143: indentation.
>>>
>>> cd_detect/Makefile:
>>> 27: CFLAGS +=
>>>
>>> cd_detect.c:
>>> 134: meida => media
>>>
>>> 639: Please add a comment here.
>>>
>>> dmi/Makefile: OK
>>>
>>> dmi/i386/Makefile:
>>> 27: CFLAGS +=
>>>
>>> i386/bios_info.c: OK
>>>
>>> cpuid.s:
>>> This file really needs comments. (I know, after Nevada Dock putback...)
>>>
>>> cpuid_info.c:
>>> 103 - 107: can be optimized to:
>>> while (isblank(cpu_name[i])) {
>>> i++;
>>> }
>>>
>>> cpuid_info.h: OK
>>>
>>> dmi/i386/dmi.c:
>>>
>>> 77: This comment is confusing. I think you mean:
>>> "If not NULL, start looking from this node."
>>>
>>> dmi/i386/dmi.h: OK
>>>
>>> dmi/i386/dmi_info.c: OK
>>>
>>> dmi/i386/dmi_info.h: OK
>>>
>>> dmi/i386/mb_info.c: OK
>>>
>>> dmi/i386/mb_info.h: OK
>>>
>>> dmi/i386/mem_info.c: OK
>>>
>>> dmi/i386/mem_info.h: OK
>>>
>>> dmi/i386/processor_info.c: OK
>>>
>>> dmi/i386/processor_info.h: OK
>>>
>>> dmi/i386/sys_info.c: OK
>>>
>>> dmi/i386/sys_info.h: OK
>>>
>>> dmi/sparc/Makefile:
>>> 27: CFLAGS +=
>>>
>>> dmi/sparc/dmi_info.c:
>>> 197, 221,234: PRINTF("Invalid int/uint/float size\n");
>>>
>>> 257: PRINTF("Unknown type\n");
>>>
>>> 845-874 is the same as 736-765: Please make a common function.
>>>
>>> 1063: Please add a comment on why get_phy_mem_size() is called or
>>> sysconf() is called. Without knowing the reason, I would just use
>>> sysconf() as it is easier.
>> I added the comments in file:
>> /*
>> * prefer get actual installed memory size from SMBIOS,
>> * if fail to get physical memory size(size=0),
>> * get memory size from syscall sysconf, it is not memory
>> * physical size, it is system can used memory size.
>> */
> OK
>>>
>>> dmi/sparc/dmi_info.h: OK
>>>
>>> find_driver/Makefile:
>>> 27: CFLAGS +=
>>>
>>> find_driver.c:
>>> 58-59: split a line like this:
>>> printf ("this is "
>>> "a long line");
>>> so that you can indent the second line properly.
>>>
>>> 257, 310-317:
>>> 257: Driver_status is not necessary.
>>> 310: Not needed
>>> 311, 314, 317: replace these lines similar to:
>>> if (tmparray[2] == 'T') {
>>> or use a switch statement:
>>> switch (tmparray[2]) :
>>> case 'T':
> Please make this (above) change.
for accurately search, do a string compare operation and "tmparray"
point to a char string.
so I'm not change this code.
>>>
>>> hd_detect/Makefile:
>>> 27: CFLAGS +=
>>>
>>> disk_info.c:
>>>
>>> 62: Based on the old version of the file, I think rqbuf for scsi
>>> commands can be 32 chars in size.
>>> 62: Please #define RQBUF_SIZE 255, then use this #define on 62, 83, 109
>>> Does scsi_cmd zero out rq_buf before it fills it in?
> I wasn't clear here. Can commands be 32 chars in size? I think they
> can. If yes, please change RQBUF_SIZE to 32.
>
> Thanks for #define RQBUF_SIZE.
OK, changed to 32.
>>>
>>> 79-81, 94-96: Can make one command:
>>> ucmd.uscsi_flags = USCSI_ISOLATE | USCSI_SILENT | USCSI_READ;
>>>
>>> hd_detect.c:
>>>
>>> 274: REMOVE_MEIDA -> REMOVE_MEDIA
>>> (needs to be changed also in hd_detect.h)
>>>
>>> 343, 402: Hardcoded value 15
>>> Use sizeof(CD_MINOR_TYPE) or just regular strcmp
>>>
>>> 366-367: inconsistent indentation of arguments relative to other
>>> functions in this module.
>>>
>>> I suggest taking the code at 505-516 and putting it above the
>>> switch. Something like :
>>>
>>> switch (c){
>>> case 'l':
>>> case 'r':
>>> case 'c':
>>> root_node = di_init(....)
>>> ...
>>> break;
>>> default:
>>> break;
>>> }
>>>
>>> switch (c) {
>>> The old switch code less the code moved to above.
>>> }
>>>
>>> hd_detect.h (not part of this webrev, but part of the last one):
>> I had remove hd_detect.h, and merge it into libddudev.h
> hd_detect.h needs to be removed from the gate. I just brought over a
> fresh child, and that file is still present.
Removed
>>>
>>>
>>> 47: MEIDA -> MEDIA
>>>
>>> mpath.c:
>>>
>>> 64: double (())
>>>
>>> 168-169: indentation. Note, you can split lines on a "."
>>>
>>> mpath.h:
>>>
>>> My comment about include files in include files still stands, but OK
>>>
>>> libddudev.h:
>>>
>>> I don't think these are used anywhere...
>>> #define PROP_DEV_TYPE "device_type"
>>> #define PROP_CPU_BRAND "brand-string"
>>> #define PROP_CPU_CLOCK "clock-frequency"
>>> #define PROP_CPU_MHZ "cpu-mhz"
>>> #define PROP_INIT_PORT "initiator-port"
>>>
>>> The following is not used, except in hd_detect.c. hd_detect.c
>>> includes hd_detect.h where it is also defined.
>>> #define BLOCK_TYPE
>>> #define CD_MINOR_TYPE
>>> #define REMOVE_MEIDA
>> These used in hd_detect.c, I removed hd_detect.h and merge it into
>> libddudev.h
> Normally I would say that if only one file will be using a #define, to
> put that #define in the file using it. However, it seems the
> #define's here are general, so OK to put them into a common header file.
>>>
>>> Not used anywhere:
>>> USB_VENDOR_NAME
>>> ... but maybe it is included out of completeness as USB_PRODUCT_NAME
>>> is used?
>>>
>>> lib/Makefile:
>>>
>>> 27 CFLAGS = -m32 -Kpic
>>> (can this be +=?)
>>>
>>> lib/libddudev.c:
>>>
>>> scsi_cmd(): A call can take potentially 30 seconds + the calls to
>>> the scsi ioctl. If scsi_cmd() is called in a loop, this time can
>>> really add up. Can we time out and return sooner?
>> Since the retries times and wait time adjust many times based on user
>> and tester feedback, so I want keep it.
> OK.
>
> I missed the media_event Makefile. Please remove the DBX reference
> there, as with the other Makefiles.
>
> Thanks again for making the changes.
>
> Thanks,
> Jack
>>>
>>> Thanks,
>>> Jack
>>>
>>> On 03/03/10 04:30, Bill Yan wrote:
>>>> Thanks a lot for the code review. We've updated the code based on
>>>> the feedback and really appreciate if you could review it again.
>>>> The following is the webrev URL:
>>>>
>>>> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev_20100303/
>>>>
>>>>
>>>> Thanks,
>>>> Bill
>>>> Bill Yan ???:
>>>>
>>>>> Here is the code review for Device Driver Utility(DDU) changes for
>>>>> Driver Update.
>>>>>
>>>>> Driver Update will allow the installer to install packages of needed
>>>>> drivers in its own boot environment before performing the
>>>>> installation,
>>>>> and then install those same packages on the target.
>>>>> DDU provides 3 methods for user to install the missing driver
>>>>> package:
>>>>>
>>>>> 1. GUI mode
>>>>> On live-cd mode, user install the missing driver package by DDU
>>>>> with GUI
>>>>> 2. Text mode
>>>>> User install the missing driver package by DDU test mode during
>>>>> the text
>>>>> installation.
>>>>> 3. AI mode
>>>>> DDU provides the common library for AI to install missing driver
>>>>> package
>>>>> during the AI installation.
>>>>>
>>>>>
>>>>>
>>>>> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_UI_webrev/
>>>>>
>>>>>
>>>>> DDU GUI, start with ddu.py
>>>>>
>>>>> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_text_webrev/
>>>>>
>>>>>
>>>>>
>>>>> DDU text mode, start with ddu-text.py
>>>>>
>>>>> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_AI_webrev/
>>>>>
>>>>>
>>>>> Common library interface used by DDU GUI, text-mode and AI
>>>>>
>>>>> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_scripts_webrev/
>>>>>
>>>>>
>>>>>
>>>>> Common library functions used by common library interface.
>>>>>
>>>>>
>>>>> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_tools_webrev/
>>>>>
>>>>>
>>>>>
>>>>> Common library functions used by common library interface.
>>>>>
>>>>>
>>>>> Please be noted the Makefiles and packaging will be rehashed when
>>>>> the DDU goes
>>>>> back to ON in April.
>>>>>
>>>>> Thanks a lot,
>>>>> Bill
>>>>>
>>>>> _______________________________________________
>>>>> caiman-discuss mailing list
>>>>> caiman-discuss at opensolaris.org
>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>>>>
>>>>>
>>>>
>>>> _______________________________________________
>>>> caiman-discuss mailing list
>>>> caiman-discuss at opensolaris.org
>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>>
>>>
>>
>
