[ofw] RE: ndinstall patch

[Smith, Stan]

Hefty, Sean wrote:
>> +#define ND_DEFAULT_PROVIDER "ibal"
>
> We can drop this.  The names are given in the provider_name array,
> which is referenced by the global index variable that defaults to 0
> (indicating IBAL). To change the default provider from ibal to
> winverbs, we only need to initialize index to 2.

OK.
Speaking windows, index should really be of the form 'g_index' or some variant.

>
>> @@ -97,14 +99,25 @@
>> static void
>> show_usage (char *progname)
>> {
>> -    printf("usage: %s\n", progname);
>> -    printf("\tRemove, install, and list OFA NetworkDirect
>> providers\n\n");
>> -    printf("\t[-[i|r] provider_name]  Install/remove the specified
>> provider\n");
>> -    printf("\t[-d]                    Install/remove debug version of
>> provider\n");
>> -    printf("\t[-l]                    list OFA ND providers\n");
>> -    printf("\tprovider_name must be one of the following:\n"); +    char
>> drv[_MAX_DRIVE]; +   char dir[_MAX_DIR];
>> +    char fname[_MAX_FNAME];
>> +    char ext[_MAX_EXT];
>> +
>> +    _splitpath(progname,drv,dir,fname,ext); // sad basename()
>> equivalent. + +      printf("usage: %s%s\n",fname,ext);
>
> This seems a little much to me...

Unless you happen to cut-n-paste to a cmd window
   cmd> C:\Documents and Settings\scsmith\My Documents\openIB-windows\SVN\gen1\branches\WOF2-1\trunk\bin\user\objfre_wlh_amd64\amd64\ndinstall.exe
Never hurts to be complete.

>
>> +    printf("\tRemove, install and list OFA NetworkDirect
>> providers\n\n"); +   printf("\t[-[i|r] [provider]] Install/remove the
>> specified/default" +                 " provider\n"); +       printf("\t[-d]
>> Install/remove debug version of provider\n"); +      printf("\t[-l]
>> list OFA ND providers\n"); + printf("\t[-q]                Suppress
>> default listing of providers\n"); +  printf("\t[-h]
>> This text\n"); +     printf("\tprovider must be one of the following
>>      names:\n");     printf("\t\tibal\n"); printf("\t\twinverbs\n");
>> +    printf("\t\t<blank> use the default ND provider '%s'\n",
>> ND_DEFAULT_PROVIDER);
>
> ND_DEFAULT_PROVIDER would become provider_name[index]

OK

>
>> }
>>
>>
>> @@ -257,6 +270,9 @@
>> {
>>      int i;
>>
>> +    if ( !name )
>> +            name = ND_DEFAULT_PROVIDER;
>
> This can be simplified to:
>
> if (!name)
>       return 0;
>
> which will leave index set to the default.  Or change these checks in
> main:
>
>       if (install || remove || get_prov_index(optarg)) {
>
> to
>
>       if (install || remove || (optarg && get_prov_index(optarg))) {
>
>> +
>>      for (i = 0; i < MAX_INDEX; i++) {
>>              if (!_stricmp(provider_name[i], name)) {
>>                      index = i;
>> @@ -269,12 +285,14 @@
>> int __cdecl main (int argc, char *argv[])
>> {
>>      int ret=0, op;
>> -    int install = 0, remove = 0, debug = 0;
>> +    int install = 0, remove = 0, debug = 0, quiet = 0;
>
> It seems like quiet could become a global value to suppress all
> output.  Now that the program returns a proper status value, that can
> be used in place of displayed output.
>
> This would imply replacing all printf statements with a macro that
> checks quiet before displaying any output.
>
> - Sean

Frankly I like the normally verbose output you choose.  Generally the 1st thing one does after installing
 a service provider is to see if it actually was installed, hence the default list output is fine.
Only in the case of running ndinstall.exe from the WinOF installer environment is the verbose output overkill.