From: Phil W. <ph...@su...> - 2002-12-11 00:12:10
|
Hi, I've implemented the new options in smartctl and committed the code. Don't expect it to work perfectly yet! The manpage, usage message, etc. still refer to the old options -- check the README for a complete listing of the new options. Bruce, I added a member called checksumignore to ataprintmain_s. This is set to TRUE if the user gives --badsum=ignore and is FALSE otherwise, but it doesn't actually have any effect yet -- I'm assuming you don't want me to add code for that. Phil |
From: Bruce A. <ba...@gr...> - 2002-12-11 06:34:48
|
Hi Phil, > I've implemented the new options in smartctl and committed the code. > Don't expect it to work perfectly yet! The manpage, usage message, > etc. still refer to the old options -- check the README for a complete > listing of the new options. OK, that's a good start. I'll have a look at the code and see if I can find any problems. > Bruce, I added a member called checksumignore to ataprintmain_s. Thanks. I tried to design things so one could add new members here without having any other effect on the code. > This is set to TRUE if the user gives --badsum=ignore and is FALSE > otherwise, but it doesn't actually have any effect yet -- I'm assuming > you don't want me to add code for that. Please go ahead and add the necessary code. I would probably just do this in ataprint.c: void checksumwarning(const char *string){ // user has asked us to ignore checksum errors if (con->checksumignore) return; pout("Warning! %s error: invalid SMART checksum.\n",string); // user has asked us to fail on checksum errors if (con->checksumfail) exit(FAILSMART); return; } but if you have a better way, please go ahead. Cheers, Bruce |
From: Bruce A. <ba...@gr...> - 2002-12-11 08:14:27
|
Hi Phil, A few minor comments on your code: (1) Please update the Usage() function so that it prints the correct names of the short/long options. (2) Could you perhaps modify the badarg branches so that they provide an error message that says what the valid options are? I am thinking of trying to improve the readability of this: [root@ballen sm5]# ./smartctl --badsum /dev/hda | more =======> INVALID ARGUMENT: /dev/hda <======= [root@ballen sm5]# ./smartctl --badsum hellobruce /dev/hda | more =======> INVALID ARGUMENT: hellobruce <======= For example: if (!strcmp(optarg,"ata")) { tryata = TRUE; tryscsi = FALSE; } else if (!strcmp(optarg,"scsi")) { tryata = TRUE; tryscsi = FALSE; } else { printf("Valid options to ... are "ata" and "scsi\n") badarg = TRUE; } It would be nice if there were a clever way of doing this, for example by passing the bad option to Usage() so that one didn't need to keep putting the same strings into different parts of the code. Can you think of a clean way to do this? (3) I would be grateful if you could get your editor to use spaces instead of tabs, eg in smartctl.h. It makes the formatting the same for everyone, regardless of their tab stop settings. Also makes the code look nicer in the CVS web interface tools. (4) We may want to promote this structure: struct { int n; char *option; } vendorattribute; to atacmds.h, give it a typedef. In the long run we may want to use these things in smartd as well. Cheers, Bruce |
From: Bruce A. <ba...@gr...> - 2002-12-11 09:11:04
|
Hi Phil, last comment on the code... vendorattribute.option = strdup(optarg); is cleaner than: vendorattribute.option = (char *)malloc(strlen(optarg)+1); and we really should check that it's not null... Here is another case that I would like to add soon: 220,temperature since often attribute 220 is temperature rather than "Disk_Shift": see ataPrintSmartAttribName(). This latter change should ideally affect all calls to ataPrintSmartAttribName(); I need to think of a clean way to do this that can also be used in smartd. I have a suggestion regarding this, and your comment below: } else { // Should handle this better badarg = TRUE; } free(vendorattribute.option); In thinking this over, I think it might be wise to do it all a bit differently. Put this in to atacmds.c (with a corresponding declaration in atacmds.h) char *validcases[]={ "9,minutes", "220,temperature", "123,whoknows", NULL }; Then in smartctl ParseOpts(): for (i=0; validcases[i] && strcmp(optarg,validcases[i]); i++); if (!validcases[i]){ // case not found printf("Case %s not recognized. Allowed options are:\n",optarg); for (i=0; validcases[i] && strcmp(optarg,validcases[i]); i++) printf("%s\n",validcases[i]); exit(); } switch (i) { case 0: con->smart009minutes=TRUE; break; case 1: con->smart220temperature=TRUE; break; case 2: con->whoknowswhat=TRUE; break; ... default: printf("smartctl does not recognize option %s although it in is the code base -- please contact the developers.... exit() } I doubt we will every have more than a dozen separate cases to consider, so I think this will keep it pretty clean. What do you think? It would also make it easier to incorporate these vendor peculiarities into smartd. Bruce |
From: Phil W. <ph...@su...> - 2002-12-11 19:05:43
|
Hi Bruce, > last comment on the code... > > vendorattribute.option = strdup(optarg); > > is cleaner than: > > vendorattribute.option = (char *)malloc(strlen(optarg)+1); Well, it looks nicer, but I don't really like the idea of making a copy of a string just to overwrite it. I know, I'm being silly :-) > and we really should check that it's not null... I did mean to add that. Really! > In thinking this over, I think it might be wise to do it all a > bit differently. Put this in to atacmds.c (with a corresponding > declaration in atacmds.h) > > char *validcases[]={ > "9,minutes", > "220,temperature", > "123,whoknows", > NULL > }; > > Then in smartctl ParseOpts(): > > for (i=0; validcases[i] && strcmp(optarg,validcases[i]); i++); > > if (!validcases[i]){ > // case not found > printf("Case %s not recognized. Allowed options are:\n",optarg); > for (i=0; validcases[i] && strcmp(optarg,validcases[i]); i++) > printf("%s\n",validcases[i]); > exit(); > } > > switch (i) { > case 0: > con->smart009minutes=TRUE; > break; > case 1: > con->smart220temperature=TRUE; > break; > case 2: > con->whoknowswhat=TRUE; > break; > ... > > default: > printf("smartctl does not recognize option %s although it in is > the code base -- please contact the developers.... > exit() > } > > > I doubt we will every have more than a dozen separate cases to consider, > so I think this will keep it pretty clean. What do you think? That's looks like a good way of doing it. I'll do that. Thanks, Phil |
From: Bruce A. <ba...@gr...> - 2002-12-11 19:12:42
|
Hi Phil, > > vendorattribute.option = strdup(optarg); > > is cleaner than: > > vendorattribute.option = (char *)malloc(strlen(optarg)+1); > > Well, it looks nicer, but I don't really like the idea of making a copy of a > string just to overwrite it. I know, I'm being silly :-) In fact this is *exactly* why I started thinking about it. My first thought was "we don't need a copy!". But then I couldn't see a clean way of doing it... until I thought of just eliminating the sscanf(). > > I doubt we will every have more than a dozen separate cases to consider, > > so I think this will keep it pretty clean. What do you think? > > That's looks like a good way of doing it. I'll do that. OK, good, I'm glad you like it. We even get helpful error messages for free... Bruce |
From: Phil W. <ph...@su...> - 2002-12-11 19:27:52
|
Hi Bruce, > > > vendorattribute.option = strdup(optarg); > > > is cleaner than: > > > vendorattribute.option = (char *)malloc(strlen(optarg)+1); > > > > Well, it looks nicer, but I don't really like the idea of making a copy of a > > string just to overwrite it. I know, I'm being silly :-) > > In fact this is *exactly* why I started thinking about it. My first > thought was "we don't need a copy!". But then I couldn't see a clean way > of doing it... until I thought of just eliminating the sscanf(). I used sscanf() so that a user could do this: $ smartctl -v 009,minutes -a /dev/hda or even this: $ smartctl -v ' 9 , minutes ' -a /dev/hda and it would still work. Though it doesn't seem to work if there is a space between the number and the comma. Phil |
From: Bruce A. <ba...@gr...> - 2002-12-11 21:05:45
|
Hi Phil, > I used sscanf() so that a user could do this: > > $ smartctl -v 009,minutes -a /dev/hda > > or even this: > > $ smartctl -v ' 9 , minutes ' -a /dev/hda > > and it would still work. Though it doesn't seem to work if there is a space > between the number and the comma. Let's keep it simple, and require the user to enter -v 9,minutes and nothing else. If they get it wrong, they'll get a nice error message... Cheers, Bruce |
From: Phil W. <ph...@su...> - 2002-12-11 23:43:49
|
Hi Bruce, > > I used sscanf() so that a user could do this: > > > > $ smartctl -v 009,minutes -a /dev/hda > > > > or even this: > > > > $ smartctl -v ' 9 , minutes ' -a /dev/hda > > > > and it would still work. Though it doesn't seem to work if there is a space > > between the number and the comma. > > Let's keep it simple, and require the user to enter -v 9,minutes and > nothing else. If they get it wrong, they'll get a nice error message... Well, OK. Really, I think the only thing that bothers me about this is the idea that a user is providing a number-string pair and the stupid program's just treating it like a common or garden string! :-) Phil |
From: Phil W. <ph...@su...> - 2002-12-11 19:02:58
|
Hi Bruce, > (1) Please update the Usage() function so that it prints the correct names > of the short/long options. I'd not forgotten. I committed the code last night for the sake of there being something for others to play with earlier rather than later. It wasn't meant to be finished in any way. > (2) Could you perhaps modify the badarg branches so that they provide > an error message that says what the valid options are? I am thinking > of trying to improve the readability of this: > > [root@ballen sm5]# ./smartctl --badsum /dev/hda | more > =======> INVALID ARGUMENT: /dev/hda <======= > > [root@ballen sm5]# ./smartctl --badsum hellobruce /dev/hda | more > =======> INVALID ARGUMENT: hellobruce <======= > > For example: > > if (!strcmp(optarg,"ata")) { > tryata = TRUE; > tryscsi = FALSE; > } else if (!strcmp(optarg,"scsi")) { > tryata = TRUE; > tryscsi = FALSE; > } else { > printf("Valid options to ... are "ata" and "scsi\n") > badarg = TRUE; > } > > It would be nice if there were a clever way of doing this, for example by > passing the bad option to Usage() so that one didn't need to keep putting > the same strings into different parts of the code. Can you think of a > clean way to do this? Like with the option names, macros might help slightly, but I'll try to think of something better. > (3) I would be grateful if you could get your editor to use spaces instead > of tabs, eg in smartctl.h. It makes the formatting the same for everyone, > regardless of their tab stop settings. Also makes the code look nicer in > the CVS web interface tools. Whoops! I thought I'd been careful. I'll look into it and at the least I'll make sure I de-tab files before committing. > (4) We may want to promote this structure: > > struct { > int n; > char *option; > } vendorattribute; > > to atacmds.h, give it a typedef. In the long run we may want to use > these things in smartd as well. OK, I'll do that. Thanks, Phil |
From: Bruce A. <ba...@gr...> - 2002-12-11 19:09:44
|
Hi Phil, > > (1) Please update the Usage() function so that it prints the correct names > > of the short/long options. > > I'd not forgotten. I committed the code last night for the sake of > there being something for others to play with earlier rather than > later. It wasn't meant to be finished in any way. OK! > > (2) Could you perhaps modify the badarg branches so that they provide > > an error message that says what the valid options are? I am thinking > > of trying to improve the readability of this: > > > > [root@ballen sm5]# ./smartctl --badsum /dev/hda | more > > =======> INVALID ARGUMENT: /dev/hda <======= > > > > [root@ballen sm5]# ./smartctl --badsum hellobruce /dev/hda | more > > =======> INVALID ARGUMENT: hellobruce <======= > > > > For example: > > > > if (!strcmp(optarg,"ata")) { > > tryata = TRUE; > > tryscsi = FALSE; > > } else if (!strcmp(optarg,"scsi")) { > > tryata = TRUE; > > tryscsi = FALSE; > > } else { > > printf("Valid options to ... are "ata" and "scsi\n") > > badarg = TRUE; > > } > > > > It would be nice if there were a clever way of doing this, for example by > > passing the bad option to Usage() so that one didn't need to keep putting > > the same strings into different parts of the code. Can you think of a > > clean way to do this? > > Like with the option names, macros might help slightly, but I'll try > to think of something better. One way we could do this is by passing the short option, or a pointer to it, to Usage(). If Usage() got NULL it would print the same error message it does now. Whereas if it got an argument, it would just print the usage line for that argument alone. But I haven't actually tried to code it so I don't know if it would work well... there may be other, better ways to do this. > Whoops! I thought I'd been careful. I'll look into it and at the least I'll > make sure I de-tab files before committing. Thanks! Cheers, Bruce |
From: Phil W. <ph...@su...> - 2002-12-11 19:37:29
|
Hi Bruce, > One way we could do this is by passing the short option, or a pointer to > it, to Usage(). If Usage() got NULL it would print the same error message > it does now. Whereas if it got an argument, it would just print the usage > line for that argument alone. I don't like the idea of overloading Usage(), so I'd rather use a separate function, but otherwise it seems like a reasonable way of handling invalid arguments. Phil |
From: Bruce A. <ba...@gr...> - 2002-12-11 21:07:55
|
> I don't like the idea of overloading Usage(), so I'd rather use a separate > function, but otherwise it seems like a reasonable way of handling invalid > arguments. OK - let's not overload Usage() if you don't want to. Cheers, Bruce |