From: Phil W. <ph...@su...> - 2003-01-30 23:30:49
|
Hi Bruce, > > That sounds like a neat way of doing it. My only comment is that I > > don't think we should assume that the user has bash. > > In fact the command is executed by system() so it uses /bin/sh. If that > doesn't exist then it uses whatever else libc provides. The point is, it > does provide an environment no matter what shell(s) your system has or > lacks. That's fine then. > I'll download some code/documentation real soon now (TM). Looking forward to seeing it. Phil |
From: Bruce A. <ba...@gr...> - 2003-01-31 03:48:23
|
> > > That sounds like a neat way of doing it. My only comment is that I > > > don't think we should assume that the user has bash. > > > > In fact the command is executed by system() so it uses /bin/sh. If that > > doesn't exist then it uses whatever else libc provides. The point is, it > > does provide an environment no matter what shell(s) your system has or > > lacks. > > That's fine then. > > > I'll download some code/documentation real soon now (TM). > > Looking forward to seeing it. I've just committed the code to CVS. See smartd.c, smartd.h, and smartd.8. man ./smartd.8 will get you started. Phil, Tim, if you have time I'd be grateful if you tested my examples from the man page. I'm running late tonight so I re-wrote them from memory in the manual and may have made small mistakes. Cheers, Bruce |
From: Bruce A. <ba...@gr...> - 2003-01-31 22:04:17
|
Hi Phil, I've revised the man page for smartd/smartd.conf some more so that it explains how I'd like the -m option to work. The code is more or less in agreement with this, although it doesn't complain if the user sets -M exec /a/path WITHOUT -m appearing. As the manual page currently reads, this should be an error -- it's supposed to require -m but without an address. In looking through the parser (which I wrote and you revised) it appears that it may be hard to handle this case -- it might be better to always have fixed numbers of arguments to -m. Otherwise the code when fed /dev/hda -m -s on -M daily might use "-s" as the argument to -m. Could you tell me if you think it's reasonable to make the code conform to the documenation I have written? Or would we be better having a "bogus" argument to -m that doesn't correspond to a valid email address, as a signal to use a script. For example we could have -m <none> or -m <script> or -m <exec> or -m <custom> or -m <use_exec_path> since angle brackets are not allowed in email addresses. At this point I think that this might make the documentation and code simpler. What do you think?? Cheers, Bruce |
From: Bruce A. <ba...@gr...> - 2003-02-01 08:55:45
|
Hi Phil, Tim, I think that this is finally converging. I've implemented and documented the following: -m address : just sends mail using 'mail' command in environment's path -m address -M exec /a/path : sends mail using /a/path -m <nomailer> -M exec /a/path: just runs /a/path with no STDIN or command line arguments at all. In all three cases a number of environment variables are set before anything is run. I have made the manual pages consistent with the code, and I've tested the example scripts from the manual. Phil, the one request I have is that I'd like you to back out the 'double quote' interpretation from smartd.c. In other words if the user says -M exec /not/a path this is (currently, and correctly) an error If the user says -M exec "/not/a path" this is NOT currently an error, but I'd like it to be one! The error message should just say something along the standard lines of: .... path" is not a Directive. In other words, I want to go back to white-space delimited tokens only. Cheers, Bruce |
From: Phil W. <ph...@su...> - 2003-02-01 13:35:53
|
Hi Bruce, > Phil, the one request I have is that I'd like you to back out the 'double > quote' interpretation from smartd.c. In other words if the user says > -M exec /not/a path > this is (currently, and correctly) an error > > If the user says > -M exec "/not/a path" > this is NOT currently an error, but I'd like it to be one! The error > message should just say something along the standard lines of: > .... path" is not a Directive. > > In other words, I want to go back to white-space delimited tokens only. No problem (but it won't happen until tomorrow at least, I'm afrad). Is there an easy way using CVS to back my changes out without breaking all of your later changes, or should I do it 'by hand'? Phil |
From: Bruce A. <ba...@gr...> - 2003-02-03 00:35:57
|
> > Phil, the one request I have is that I'd like you to back out the 'double > > quote' interpretation from smartd.c. In other words if the user says > > -M exec /not/a path > > this is (currently, and correctly) an error > > > > If the user says > > -M exec "/not/a path" > > this is NOT currently an error, but I'd like it to be one! The error > > message should just say something along the standard lines of: > > .... path" is not a Directive. > > > > In other words, I want to go back to white-space delimited tokens only. > > No problem (but it won't happen until tomorrow at least, I'm afrad). > Is there an easy way using CVS to back my changes out without breaking > all of your later changes, or should I do it 'by hand'? I don't know how to make CVS do this, but I think it may be possible. Without investigating CVS, here's what I might try: do: cvs diff -r 1.X -r 1.X+1 where revisions X and X+1 are the versions just before and just after you committed your changes. Then, take the diff output and apply it using "patch" to the latest version of the code. It *might* complain because the line numbering no longer agrees, but might be able to find the right location from context information. Otherwise, you probably ought to back out the changes by hand, just holding the diff output in one hand and the keyboard in the other (:-). Cheers, Bruce |
From: Bruce A. <ba...@gr...> - 2003-02-03 04:00:10
|
Hi Phil, > > > In other words, I want to go back to white-space delimited tokens only. > > > > No problem (but it won't happen until tomorrow at least, I'm afrad). > > Is there an easy way using CVS to back my changes out without breaking > > all of your later changes, or should I do it 'by hand'? Just to be clear, I definitely don't want you to back out your changes regarding the additional structures in conf-> for the different options. Just the changes that make it possible to parse a double-quote delimited string. So I doubt that doing this automatically is an option. I think you'll have to do it the old fashioned way... Cheers, Bruce |
From: Phil W. <ph...@su...> - 2003-02-03 19:25:01
|
Hi Bruce, > > > > In other words, I want to go back to white-space delimited tokens only. > > > > > > No problem (but it won't happen until tomorrow at least, I'm afrad). > > > Is there an easy way using CVS to back my changes out without breaking > > > all of your later changes, or should I do it 'by hand'? > > Just to be clear, I definitely don't want you to back out your changes > regarding the additional structures in conf-> for the different > options. Just the changes that make it possible to parse a double-quote > delimited string. > > So I doubt that doing this automatically is an option. I think you'll > have to do it the old fashioned way... OK, that's done, and like you suggested a while back, I added an info message for the case that -M exec appears more than once on a line. Phil |
From: Bruce A. <ba...@gr...> - 2003-02-03 19:55:08
|
Hi Phil, > > Just to be clear, I definitely don't want you to back out your changes > > regarding the additional structures in conf-> for the different > > options. Just the changes that make it possible to parse a double-quote > > delimited string. > > > > So I doubt that doing this automatically is an option. I think you'll > > have to do it the old fashioned way... > > OK, that's done, and like you suggested a while back, I added an info message > for the case that -M exec appears more than once on a line. Thanks for doing this. I'm going to work on one additional change that Tim requested. The idea is that if: DEVICESCAN is present in smartd.conf, to allow it to take arguments that apply to all the disks that are found, eg: DEVICESCAN -m root@home Phil, I have an additional request -- could you please update the blurb at the end of README to explain the multiple -M options eg -M daily -M test rather than -M test,daily? This README blurb is meant mostly for 5.0.x users so we should tell them how the latest version of 5.1 works. Cheers, Bruce |
From: Bruce A. <ba...@gr...> - 2003-02-06 19:02:59
|
Hi Phil, Tim, I'm getting close to issuing another smartmontools release. But I wanted to ask the two of you for a hand in testing the code and documentation that I've checked into CVS before I generate a new release. I've made three changes. (Tim, if I remember the history correctly, the first two of these are in response to suggestions that you made.) First, I've implemented and documented the /etc/smartd.conf Directives: -m ADDRESS -M exec /a/path and -m <nomailer> -M exec /another/path Second, I've make the DEVICESCAN Directive capable of taking all the same Directives as a device path can take. So you can now say DEVICESCAN -m tim@work or DEVICESCAN -d ata -H tim@home or DEVICESCAN -m <nomailer> -M /another/path Third, I've added a command-line option to smartd: -c --checkonce which is meant for use by Distribution Installer writers. It makes smartd run in foreground mode, and exit with zero status after a sucessful pass through registering/checking the disks. It can be used to automatically verify that smartd "works" on a given system. I'm confident in the third change, but I wanted to ask the two of you to do a bit of testing to see that the first two changes are correct. I also wanted your input on whether I should add a subdirectory to sm5/ to include with the distribution that includes example scripts. I've put two short examples in to the man page for smartd. Finally, Tim, if you have any further ideas on how to make a .spec file do the build/install under the current directory without having to mess with the users .rpmrc files, I'd be interested in putting this into the Makefile. Cheers, Bruce |
From: Bruce A. <ba...@gr...> - 2003-02-06 19:26:40
|
> or > DEVICESCAN -d ata -H tim@home I should have written: DEVICESCAN -d ata -H -m tim@home Cheers, Bruce |
From: Phil W. <ph...@su...> - 2003-02-08 20:18:05
|
Hi Bruce, > I'm getting close to issuing another smartmontools release. But I wanted > to ask the two of you for a hand in testing the code and documentation > that I've checked into CVS before I generate a new release. > > ... > > First, I've implemented and documented the /etc/smartd.conf Directives: > -m ADDRESS -M exec /a/path > and > -m <nomailer> -M exec /another/path The documentation looks fine and smartd worked for me as I expected. My only comment is that perhaps the newline should be removed from the SMARTD_TFIRST string. > Second, I've make the DEVICESCAN Directive capable of taking all the same > Directives as a device path can take. So you can now say > DEVICESCAN -m tim@work > or > DEVICESCAN -d ata -H tim@home > > or > DEVICESCAN -m <nomailer> -M /another/path This worked fine for me as well, but since I've only got one SMART device on my machine it wasn't the best possible test. > I also wanted your input on whether I should add a subdirectory to sm5/ to > include with the distribution that includes example scripts. I've put two > short examples in to the man page for smartd. That sounds good to me. Maybe the example smartd.conf should go in there as well. Phil |
From: Bruce A. <ba...@gr...> - 2003-02-09 05:53:57
|
Hi Phil, > > I'm getting close to issuing another smartmontools release. But I wanted > > to ask the two of you for a hand in testing the code and documentation > > that I've checked into CVS before I generate a new release. > > > > First, I've implemented and documented the /etc/smartd.conf Directives: > > -m ADDRESS -M exec /a/path > > and > > -m <nomailer> -M exec /another/path > > The documentation looks fine and smartd worked for me as I expected. > My only comment is that perhaps the newline should be removed from the > SMARTD_TFIRST string. Thanks for spotting this -- I missed it. I'll do what you suggest. In fact I added a function to "utility" to print timezone info as well; I'll remove the newline and add the timezone. > > > Second, I've make the DEVICESCAN Directive capable of taking all the same > > Directives as a device path can take. So you can now say > > DEVICESCAN -m tim@work > > or > > DEVICESCAN -d ata -H tim@home > > > > or > > DEVICESCAN -m <nomailer> -M /another/path > > This worked fine for me as well, but since I've only got one SMART device on > my machine it wasn't the best possible test. > > > I also wanted your input on whether I should add a subdirectory to sm5/ to > > include with the distribution that includes example scripts. I've put two > > short examples in to the man page for smartd. > > That sounds good to me. Maybe the example smartd.conf should go in there as > well. OK, I'd like to see what TIm says about this. I sort of like the idea of a directory for some -M exec scripts, but I don't think that smartd.conf belongs there... Thanks very much for taking a close look. Tim, will you be able to do some testing too? Cheers, Bruce |
From: Bruce A. <ba...@gr...> - 2003-02-09 21:16:16
|
Hi Phil, > > The documentation looks fine and smartd worked for me as I expected. > > My only comment is that perhaps the newline should be removed from the > > SMARTD_TFIRST string. > > Thanks for spotting this -- I missed it. > > I'll do what you suggest. In fact I added a function to "utility" to > print timezone info as well; I'll remove the newline and add the timezone. I've now done what you suggested: -- SMARTD_TFIRST no longer has a terminating newline. -- I've also added the timezone inforrmation to it, eg Sun Feb 9 14:58:19 2003 CST rather than Sun Feb 9 14:58:19 2003 -- I've added an additional environment variable SMARTD_TFIRSTEPOCH which is the unix epoch (seconds since Jan 1 1970) of SMARTD_TFIRST. This revised version is in CVS. Cheers, Bruce |
From: Phil W. <ph...@su...> - 2003-02-01 13:28:03
|
Hi Bruce, > I've revised the man page for smartd/smartd.conf some more so that it > explains how I'd like the -m option to work. The code is more or less in > agreement with this, although it doesn't complain if the user sets -M exec > /a/path WITHOUT -m appearing. As the manual page currently reads, this > should be an error -- it's supposed to require -m but without an address. Weren't we going to make the '-m' optional if '-M exec' is used but require an email address if '-m' is used? Did that turn out not to be a good idea when you came to implement it? > In looking through the parser (which I wrote and you revised) it appears > that it may be hard to handle this case -- it might be better to always > have fixed numbers of arguments to -m. Otherwise the code when > fed > > /dev/hda -m -s on -M daily > > might use "-s" as the argument to -m. > > Could you tell me if you think it's reasonable to make the code conform to > the documenation I have written? Or would we be better having a > "bogus" argument to -m that doesn't correspond to a valid email address, > as a signal to use a script. For example we could have > -m <none> > or > -m <script> > or > -m <exec> > or > -m <custom> > or > -m <use_exec_path> > > since angle brackets are not allowed in email addresses. > > At this point I think that this might make the documentation and code > simpler. What do you think?? I can't see any easy way of making the argument optional either. I like '-m <none>' of the examples you gave. Phil |
From: Bruce A. <ba...@gr...> - 2003-02-03 00:32:44
|
> > -m <use_exec_path> > > > > since angle brackets are not allowed in email addresses. > > > > At this point I think that this might make the documentation and code > > simpler. What do you think?? > > I can't see any easy way of making the argument optional either. I like > '-m <none>' of the examples you gave. I ended up choosing -m <nomailer> but it's trivial to change this in the code and documentation if you feel strongly about it. Bruce |
From: Phil W. <ph...@su...> - 2003-02-03 19:27:02
|
Hi Bruce, On Sun, Feb 02, 2003 at 06:32:27PM -0600, Bruce Allen wrote: > > > -m <use_exec_path> > > > > > > since angle brackets are not allowed in email addresses. > > > > > > At this point I think that this might make the documentation and code > > > simpler. What do you think?? > > > > I can't see any easy way of making the argument optional either. I like > > '-m <none>' of the examples you gave. > > I ended up choosing > -m <nomailer> > but it's trivial to change this in the code and documentation if you feel > strongly about it. No, that's fine. Phil |
From: Bruce A. <ba...@gr...> - 2003-01-31 15:14:25
|
Hi Tim, Phil, I spent some time testing my code this morning and immediately discovered two nasty things: (1) There is NO platform independent way to put double quotes (") into an nroff/troff document. Writing man pages with double quotes is very hard to do. When I displayed my man pages, some essential double quotes were missing. (2) The implementation of smartd.c that I checked into CVS last night is 'hard to use' when one tries putting a little script into the -M exec argument because of the way quote and double-quote interpretation is done by shells and by the smartd.conf parser. I thought a bit about some possible solutions. One was the following. If the argument to -M exec contains spaces, then enclose it in braces {}. The parser could then count open/close brace pairs and terminate at the correct closing brace. This makes it easy for users to include double quotes into their -M exec arguments. But after some further thought, I realized that this is actually a nightmare. We are not guaranteed about what the users shell will require, and there is no guarantee that the braces will be needed in pairs, etc. We'd end up producing a utility that could only be used after some extensive experimentation on a given system. [We've all done this before, having to reverse engineer some utility to try and pass arguments to it correctly, etc. Let's not create a similar hassle for our users.] So I now have a proposal to make things simpler and more reliable: I think we should require that the argument to -M exec is a path (a single "word" with NO embedded spaces). Let's eliminate any double quoting from smartd.conf. The original reason for the double quotes was to allow the imbedding of %s (for the drive) and other arguments after the executable. But this is now moot. By exporting environment SMARTD_* variables, we give users easy access to any of the information that they might need about the device path, type, problem, etc. Also, I have a proposed change to the syntax. I propose that we make the -m Directive mandatory. There will be two possible way to invoke it: (1) -m user@domain -M exec /path/to/executable In this case, we'll run "/path/to/executable" with the mail message on stdin, and the command-line arguments: -s "$SMARTD_SUBJECT" $SMARTD_MAILER This is the syntax that will be used when people want to specify an alternate path to /bin/mail, for example. Or when they want to replace /bin/mail with a customized script, but still want trivial access to the mail message that would ordinarily be sent. (2) -m -M exec /path/to/executable Note that THERE IS NO ARGUMENT to -m. In this case we'll run /path/to/executable with nothing on stdin, and no command line arguments. If the user wants to provide command line arguments to their executable, they can wrap it in a script if needed. I think that this would give us a consistent, simple and predictable extenral interface. And I think users could work within it, without much difficulty. It still allows a three-line solution to the original issue that Tim raised. Comments? Cheers, Bruce |
From: Bruce A. <ba...@gr...> - 2003-01-31 16:30:29
|
Phil, Tim, I've checked in revised smartd.8 and smartd.conf.5 files into CVS. Please have a look at these to see how I am proposing changing the -m and -M exec syntax. Note: I have not yet modified the code to accomodate this, but will do so unless I hear strong objections. Phil, if we go this route I suggest backing out the "double quote" interpretation version of strtok(). I'd also be grateful for your help in modifying the error messages for -m (since now the argument is optional) and -M exec. -m (with no ADDRESS) argument should ONLY give an error and exit if -M exec PATH is absent. -M exec PATH should give an error and exit if PATH is not a single word (with the standard definition of white space). Cheers, Bruce On Fri, 31 Jan 2003, Bruce Allen wrote: > Hi Tim, Phil, > > I spent some time testing my code this morning and immediately discovered > two nasty things: > > (1) There is NO platform independent way to put double quotes (") into an > nroff/troff document. Writing man pages with double quotes is very > hard to do. When I displayed my man pages, some essential double > quotes were missing. > > (2) The implementation of smartd.c that I checked into CVS last night > is 'hard to use' when one tries putting a little script into the -M > exec argument because of the way quote and double-quote interpretation > is done by shells and by the smartd.conf parser. > > I thought a bit about some possible solutions. One was the following. > If the argument to -M exec contains spaces, then enclose it in braces {}. > The parser could then count open/close brace pairs and terminate at the > correct closing brace. This makes it easy for users to include double > quotes into their -M exec arguments. > > But after some further thought, I realized that this is actually a > nightmare. We are not guaranteed about what the users shell will require, > and there is no guarantee that the braces will be needed in pairs, etc. > We'd end up producing a utility that could only be used after some > extensive experimentation on a given system. [We've all done this before, > having to reverse engineer some utility to try and pass arguments to it > correctly, etc. Let's not create a similar hassle for our users.] > > So I now have a proposal to make things simpler and more reliable: > > I think we should require that the argument to -M exec is a path (a single > "word" with NO embedded spaces). Let's eliminate any double quoting from > smartd.conf. > > The original reason for the double quotes was to allow the imbedding of %s > (for the drive) and other arguments after the executable. But this is now > moot. By exporting environment SMARTD_* variables, we give users easy > access to any of the information that they might need about the device > path, type, problem, etc. > > Also, I have a proposed change to the syntax. I propose that we make the > -m Directive mandatory. There will be two possible way to invoke it: > > (1) -m user@domain -M exec /path/to/executable > > In this case, we'll run "/path/to/executable" with the mail message on > stdin, and the command-line arguments: > -s "$SMARTD_SUBJECT" $SMARTD_MAILER > > This is the syntax that will be used when people want to specify > an alternate path to /bin/mail, for example. Or when they > want to replace /bin/mail with a customized script, but still want > trivial access to the mail message that would ordinarily be sent. > > (2) -m -M exec /path/to/executable > > Note that THERE IS NO ARGUMENT to -m. > > In this case we'll run /path/to/executable with nothing on stdin, > and no command line arguments. > > If the user wants to provide command line arguments to their executable, > they can wrap it in a script if needed. > > I think that this would give us a consistent, simple and predictable > extenral interface. And I think users could work within it, without much > difficulty. It still allows a three-line solution to the original issue > that Tim raised. > > Comments? > > Cheers, > Bruce > > |
From: Guido G. <ag...@si...> - 2003-01-31 16:35:46
|
Hi Bruce, On Fri, Jan 31, 2003 at 09:10:01AM -0600, Bruce Allen wrote: > If the user wants to provide command line arguments to their executable, > they can wrap it in a script if needed. > > I think that this would give us a consistent, simple and predictable > extenral interface. And I think users could work within it, without much > difficulty. It still allows a three-line solution to the original issue > that Tim raised. > > Comments? I still don't grasp why -M exec is really needed? With SMARTD_MAILER (which is *really* nice) there's plenty of room for own scripts and '-M exec' looks overly complicated to use to me. Regards, -- Guido |
From: Bruce A. <ba...@gr...> - 2003-01-31 16:59:48
|
Hi Guido, > I still don't grasp why -M exec is really needed? With SMARTD_MAILER > (which is *really* nice) there's plenty of room for own scripts and '-M > exec' looks overly complicated to use to me. The problem with using one environment variable (SMARTD_MAILER) is that the user might want to have different scripts for different disks. This could be done with a switch statement in the script, but I'd rather have SMARTD_MAILER be output rather than input from smartd. Please, have a look at the revised man pages in CVS and then tell me if you still think it's confusing. Bruce |
From: Bruce A. <ba...@di...> - 2003-01-27 20:17:33
|
Hi Jamie, > /bin/mail shouldn't ever be used by external programs, infact, > /bin/mail is a lousy piece of software that in an ideal world would > simply be removed entirely. You don't need any temp files, fork, > exec, close up shop and call it a day. It's a reasonable alternative. Are you volunteering to help implement it and then to support it? Bruce |
From: Jamie H. <ja...@au...> - 2003-01-28 08:38:31
|
Bruce Allen wrote: > Hi Jamie, > > > /bin/mail shouldn't ever be used by external programs, infact, > > /bin/mail is a lousy piece of software that in an ideal world would > > simply be removed entirely. You don't need any temp files, fork, > > exec, close up shop and call it a day. > > It's a reasonable alternative. > > Are you volunteering to help implement it and then to support it? Nope, just offering advice, use it as you see fit. -- Jamie Heilman http://audible.transient.net/~jamie/ "Most people wouldn't know music if it came up and bit them on the ass." -Frank Zappa |
From: Tim S. <ts...@do...> - 2003-01-17 19:42:16
|
Hi Bruce On Tue, 14 Jan 2003 10:20:14 -0600 (CST) Bruce Allen <ba...@di...> jiggled the electrons to say: > > At the moment the output of smartctl can't exceed a certain length, > because in the worst case it prints 5 ATA error structures of fixed > length and 21 self-test error structures of fixed length. OK - maybe I'll be less lazy and malloc/realloc to cover the future. > > Hmmm I thought that the device name was always in cfg->name. But I > remember that the pointer/info was in a couple of different places, > so perhaps it's necessary to include this device name separately as > well. I do need to study the code in more detail to understand it better :-) Thanks for pointing that out - I can look at this first and see if I've been braindead. > Easy -- let's just modify/extend the arguments to -M as I proposed in > an earlier message. > > 3 - Mail handling is still kind of tied to /bin/mail. > > Not going to get into a debate about this, save that > > perhaps unix needs a common mail-send library with a > > one off config and interfaces to libesmtp, /bin/mail, sendmail > > etc. But that's another discussion for another place. > > I tend to put full paths in everywhere, because I'm > > paranoid about root processes - just my way. Perhaps > > these could be derived at build time automagically. > > Do you like the idea of just controlling the path with an environment > variable? It's safe and easy to control from the user's point of view. I think until there's a *standard* lib call in GNU to say "just send this mail to fred@somewhere" that this is the best that can be done. I suppose /bin/mail has been the best standard to date. > > > BUGS - Very likely. I said before, I'm a perl programmer > > and I'm tainted now. Sorry. > > In looking through it, the only specific comments were: > -- cfg->name (I think...) can replace one argument. OK. > -- printf() shouldn't be used. The printout function is designed to > redirect > output to either syslog or terminal depending upon the users > command line options. I saw this in sendmail(). Likewise there is > a replacement for perror() called syserror() that "does the right > thing" with the output. Righty ho - easily fixed - thanks for that. > -- Have you looked closely at the signal handling to be sure that it > won't interfere > with the existing use/handling of USR1, HUP and TERM? I think so - I was careful to only turn off SIGPIPE and to restore the original handler if there was one. Will test to be sure. > -- might be nice to have a pipe for stderr as well, so if mail has > trouble we can > report the error back to the user. Sounds good - I'll have a go. > > > In general I thought that the code looked very clean. Thank you :-) I'm gradually getting back into C - I learnt it years ago, then didn't have so much use until recently. Best Tim -- Mr Tim J Southerwood CSG, Dept of Computing, Imperial College, London Email personal: ts...@di... work: ts...@do... Tel home: 020-866-17388 mobile: 07949-487179 work: 020-759-48392 |
From: Bruce A. <ba...@gr...> - 2003-01-17 20:15:27
|
Hi Tim, > > At the moment the output of smartctl can't exceed a certain length, > > because in the worst case it prints 5 ATA error structures of fixed > > length and 21 self-test error structures of fixed length. > > OK - maybe I'll be less lazy and malloc/realloc to cover the > future. That's probably best -- it's not that much extra work. > > Hmmm I thought that the device name was always in cfg->name. But I > > remember that the pointer/info was in a couple of different places, > > so perhaps it's necessary to include this device name separately as > > well. > > I do need to study the code in more detail to understand it better :-) > Thanks for pointing that out - I can look at this first and see if I've > been braindead. Umm, while you are at it, if you notice obvious pointer duplications that could just be eliminated, please feel free. Ultimately the data from smartd.conf should end up in cfg->whatever. For historical reasons some of it is spread around in other places, but probably not used much, or at all any more. > > > 3 - Mail handling is still kind of tied to /bin/mail. > > > Not going to get into a debate about this, save that > > > perhaps unix needs a common mail-send library with a > > > one off config and interfaces to libesmtp, /bin/mail, sendmail > > > etc. But that's another discussion for another place. > > > I tend to put full paths in everywhere, because I'm > > > paranoid about root processes - just my way. Perhaps > > > these could be derived at build time automagically. > > > > Do you like the idea of just controlling the path with an environment > > variable? > > It's safe and easy to control from the user's point of view. > I think until there's a *standard* lib call in GNU to say > "just send this mail to fred@somewhere" that this > is the best that can be done. I suppose /bin/mail has been the best > standard to date. In any case we are trying to stay at least somewhat glibc independent, so that Darwin and FreeBSD ports are less painful. So let's use environment variables SMARTDMAIL and SMARTDSMARTCTL for now. If they are null then just use /bin/mail and /usr/sbin/smartctl by default. > > -- Have you looked closely at the signal handling to be sure that it > > won't interfere > > with the existing use/handling of USR1, HUP and TERM? > > I think so - I was careful to only turn off SIGPIPE and to restore > the original handler if there was one. Will test to be sure. At some point I had written some code to replace the ugly while () { ... sleep(1) } busy loop in smartd. It does a better job of the HUP and USR1 signal handling. Perhaps I will dust that off, test it, and put it into smartd. Of course then if things break it's almost certainly my fault... > > -- might be nice to have a pipe for stderr as well, so if mail has > > trouble we can > > report the error back to the user. > > Sounds good - I'll have a go. Thanks. One of the issues that I thought about briefly was this. If /sbin/mail breaks, do I want smartd to hang? Or do I want it to fill the process table with lots of hanging mail jobs that have fork()ed off every 30 minutes and then are hanging forever, unable to complete. In the end I went for the easy solution: have smartd hang until system() returns, so if mail fails and hangs then smartd stops and hangs. Getting back the mailer errors would be an improvement on this. > > In general I thought that the code looked very clean. > > Thank you :-) I'm gradually getting back into C - I learnt it years > ago, then didn't have so much use until recently. Good and bad programming practice is practically language-independent. So I suspect the years of experience with other languages has made you a better programmer. Just keep your K&R and your W. Richard Stevens close at hand... Cheers, Bruce |