Bug 31121

There's a patch on https://bugzilla.redhat.com/show_bug.cgi?id=646715#c14, but I don't think it's correct. I've tested this and when I send empty lines, they don't trigger the problem and get logged. In fact they are not null strings, they are '\n', so TPL_STR_EMPTY() won't return TRUE on them and so that can't be the cause.

My fix sets the error before returning FALSE so that when we check the error on _tpl_log_manager_add_message(), it's set and we don't crash.

Created attachment 40835 [details] [review]
Set the error if there's no message body

(In reply to comment #1)
> There's a patch on https://bugzilla.redhat.com/show_bug.cgi?id=646715#c14, but
> I don't think it's correct. I've tested this and when I send empty lines, they
> don't trigger the problem and get logged.

You mean that when you send empty lines they don't generate an error, and do appear in the log? If so, that's the intended behaviour :)

> In fact they are not null strings,
> they are '\n', so TPL_STR_EMPTY() won't return TRUE on them and so that can't
> be the cause.

I've traced this, and they're definitely empty strings. See my gdb output in https://bugzilla.redhat.com/show_bug.cgi?id=646715#c17

> My fix sets the error before returning FALSE so that when we check the error on
> _tpl_log_manager_add_message(), it's set and we don't crash.

I don't believe this is correct. The reason being that every time somebody enters a blank line, telepathy will trigger the following in log-manager.c:

          CRITICAL ("logstore name=%s: %s. "
              "Event may not be logged properly.",
              _tpl_log_store_get_name (store), loc_error->message);

By all means look for an alternative root cause, though. If you're convinced that \n isn't supposed to be stripped, it would be worth looking for why it has been stripped. Not being personally familiar with the codebase outside debugging this one issue, I can't say for sure what the expected behaviour is. However, I have noticed that it logs a separate message per line. Stripping newlines does seem to make sense in this context.

(In reply to comment #3)
> (In reply to comment #1)
> > There's a patch on https://bugzilla.redhat.com/show_bug.cgi?id=646715#c14, but
> > I don't think it's correct. I've tested this and when I send empty lines, they
> > don't trigger the problem and get logged.
> 
> You mean that when you send empty lines they don't generate an error, and do
> appear in the log? If so, that's the intended behaviour :)

Yes... without your patch. :)

> > In fact they are not null strings,
> > they are '\n', so TPL_STR_EMPTY() won't return TRUE on them and so that can't
> > be the cause.
> 
> I've traced this, and they're definitely empty strings. See my gdb output in
> https://bugzilla.redhat.com/show_bug.cgi?id=646715#c17

They are not for me:

(gdb) print self->priv->message
$5 = (gchar *) 0x654f20 "\n"

So something on your side is stripping them. I assume you're testing with Empathy... what version? I'm using git master of empathy and tp-logger (though tp-logger 0.1.7 shouldn't make a difference).

> > My fix sets the error before returning FALSE so that when we check the error on
> > _tpl_log_manager_add_message(), it's set and we don't crash.
> 
> I don't believe this is correct. The reason being that every time somebody
> enters a blank line, telepathy will trigger the following in log-manager.c:
> 
>           CRITICAL ("logstore name=%s: %s. "
>               "Event may not be logged properly.",
>               _tpl_log_store_get_name (store), loc_error->message);
> 
> By all means look for an alternative root cause, though. If you're convinced
> that \n isn't supposed to be stripped, it would be worth looking for why it has
> been stripped. Not being personally familiar with the codebase outside
> debugging this one issue, I can't say for sure what the expected behaviour is.
> However, I have noticed that it logs a separate message per line. Stripping
> newlines does seem to make sense in this context.

Yeah. I'm not happy with comitting your fix until we know why lines are being stripped. And then if it's the logger we should just fix it, and if it's something else we'll see.

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #1)
> > > There's a patch on https://bugzilla.redhat.com/show_bug.cgi?id=646715#c14, but
> > > I don't think it's correct. I've tested this and when I send empty lines, they
> > > don't trigger the problem and get logged.
> > 
> > You mean that when you send empty lines they don't generate an error, and do
> > appear in the log? If so, that's the intended behaviour :)
> 
> Yes... without your patch. :)
> 
> > > In fact they are not null strings,
> > > they are '\n', so TPL_STR_EMPTY() won't return TRUE on them and so that can't
> > > be the cause.
> > 
> > I've traced this, and they're definitely empty strings. See my gdb output in
> > https://bugzilla.redhat.com/show_bug.cgi?id=646715#c17
> 
> They are not for me:
> 
> (gdb) print self->priv->message
> $5 = (gchar *) 0x654f20 "\n"
> 
> So something on your side is stripping them. I assume you're testing with
> Empathy... what version? I'm using git master of empathy and tp-logger (though
> tp-logger 0.1.7 shouldn't make a difference).

Yes, this issue is when using empathy. I'm using empathy, telepathy-idle and telepathy-logger from F14:
empathy-2.32.2-1.fc14.x86_64
telepathy-logger-0.1.7-1.fc14.x86_64
telepathy-idle-0.1.7-1.fc14.x86_64

In latest version we check if error is not NULL instead of return value. Not perfect but it does not crash. This error is just for logging anyways.