[collectd] notify_email plugin

[Sebastian Harl]

Hi Oleg,

On Wed, Jun 04, 2008 at 04:33:01AM +0400, Oleg King wrote:
>   I wrote a new plugin - notify_email.

Cool!

>   Patch and new file are included as attach.

I'm currently busy, but here are a few quick notes from a very quick
first glance: ;-)

First off: since you did not include a copyright header, we assume that
you've put your code under the same license as collectd itself, namely
GPL2. Since it's not really valid to just make that assumption, it would
be nice if you did confirm that ;-)

> static const char *config_keys[] =
> {
> 	"SMTPHost",
> 	"SMTPPort",
> 	"SMTPUser",
> 	"SMTPPassword",
> 	"SMTPFrom",
> 	"SMTPSubject",
> 	"EmailTo"
> };

I think, I would have left out the prefixes "SMTP" and "Email", but, for
me, it's perfectly fine to include them ;-)

> struct emaillist_s
> {
>         char *email;
>         struct emaillist_s *next;
> };
> typedef struct emaillist_s emaillist_t;
> static emaillist_t *emails = NULL;

In most cases, traversing an array is slightly more efficient than
traversing a linked list. A linked list adds the benefit of making
modifications easier which, imho, is not required in this case though.
So, I'd suggest using an array here.

> /* Callback to get username and password */
> static int authinteract (auth_client_request_t request, char **result, int fields, void *arg)
> {               
>         int i;
>         for (i = 0; i < fields; i++)
>         {
>                 if (request[i].flags & AUTH_USER)
>                         result[i] = smtp_user;
>                 else if (request[i].flags & AUTH_PASS)
>                         result[i] = smtp_password;

smtp_user and smtp_password are initialized with NULL. Did you check
that libesmtp handles that correctly?

> /* Callback to print the recipient status */
> void
> print_recipient_status (smtp_recipient_t recipient, const char *mailbox, void *arg)
[...]
> /* Callback to monitor SMTP activity */
> void
> monitor_cb (const char *buf, int buflen, int writing, void *arg)

As far as I can see, those functions should be declared static - is
there any reason not to do so?

> /* RFC822 message text MUST be with \r\n EOLs */
> static int esmtp_send (char *message_text)
> {
[...]
>     auth_client_init();
>     if ( !(session = smtp_create_session()) ) {
>         ERROR ("notify_email plugin: cannot create SMTP session");
>         return (-1);

Is it necessary to do that kind of initialization for each single
E-mail or could that be done in some init-callback as well?

>     sa.sa_handler = SIG_IGN;
>     sigemptyset (&sa.sa_mask);
>     sa.sa_flags = 0;
>     sigaction (SIGPIPE, &sa, NULL); 

collectd already ignores SIGPIPE - src/see collectd.c.

>     if (smtp_user && smtp_password) {
>         authctx = auth_create_context ();
>         auth_set_mechanism_flags (authctx, AUTH_PLUGIN_PLAIN, 0);
>         auth_set_interact_cb (authctx, authinteract, NULL);

Oh, that looks like you only register the authentication callback if
both smtp_user and smtp_password are not equal to NULL - in that case
please ignore my comment above, but please add an appropriate comment to
the callback function.

>     if ( !(message = smtp_add_message (session))) {
>         ERROR ("notify_email plugin: cannot set SMTP message");
>         return (-1);   

Don't you have to destroy the session, etc. before you return from the
function in case of an error?

>     if (smtp_from == NULL) {
>         sfree (smtp_from);
>         smtp_from = strdup(smtp_from);

Uh? What's that supposed to be? ;-)

>     } else {
>         /* Report on the success or otherwise of the mail transfer. */
>         status = smtp_message_transfer_status (message);
>         printf ("%d %s", status->code,
>                 (status->text != NULL) ? status->text : "\n");

That should be reported using collectd's logging mechanism (INFO()
should be fine in this case).

> static int notify_email_notification (const notification_t *n)
[...]
> 	sprintf (subject, smtp_subject == NULL ? DEFAULT_SMTP_SUBJECT : smtp_subject, severity, n->host);

While this is a really nice feature, it requires some input validation.
Image the user specifies more than two conversion specifications
(%somethings) - then random memory is read to fetch subsequent
parameters which is really not what we want. Also, the verification
should check if a string (%s) has been specified.

If we want to be 100% correct, we'd also have to check if the string
specified by the user is ASCII encoded, as anything else may not be used
in a subject. However, I think it's fine to just mention that in
collectd.conf(5).

Cheers,
Sebastian

-- 
Sebastian "tokkee" Harl +++ GnuPG-ID: 0x8501C7FC +++ http://tokkee.org/

Those who would give up Essential Liberty to purchase a little Temporary
Safety, deserve neither Liberty nor Safety.         -- Benjamin Franklin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20080604/0a0b04c5/attachment.pgp