[collectd] notify_email plugin

[Oleg King]

Hello, Sebastian.

> 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 ;-)

Yes, sure.

Disclaimer:
I'm not so familiar with C language, my faforite langs are
perl/php/javascript, so please forgive me for my, sometimes stupid,
mistakes.

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

May be it will use local mailer also (in future), in this case prefix
will be useful..

> 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.

Yes, agree. See "Disclaimer" :)

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

libesmtp deals with user and password only when these values are set.

>> print_recipient_status (smtp_recipient_t recipient, const char *mailbox, void *arg)
>> 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?

Yes, may be. I just do not known exact difference. See "Disclaimer" :)

>>     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?

I do not know exactly, I will try to divide it into init/send/shutdown.

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

OK, I will remove this code.

> 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.

OK.

>>     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?

Yes, this is exactly as example from libesmtp does. libesmtp has
documented approx. half of its functions, and workflow exists only in
example file. So I think nobody knows it for sure :)

>>     if (smtp_from == NULL) {
>>         sfree (smtp_from);
>>         smtp_from = strdup(smtp_from);
> Uh? What's that supposed to be? ;-)

WOW. May be it is copy/paste made by accident?.. WOW again. I'll
remove this. :)

>>     } 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).

OK.

>> 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.

As I know sprintf should not do substitutions more than number of
arguments and more then number of %something.

I'm not right?

> 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).

OK.

-- 
WBR,
 Oleg                          mailto:king2 at kaluga.ru