Unify logging in libxrdp #1738
Unify logging in libxrdp #1738
Conversation
aquesnel
force-pushed
the
unify_logging_libxrdp
branch
from
53f7571 to
2d6d249
Compare
|
Nice work! I'm not hugely familiar with this area of the code, but looking at your PR makes me realise this is well worth doing. The only comment I've got is similar to one I made on #1738. Where there's currently an output produced by Please come back to me on that if you've got a strong opinion on this. Also there are a few commented out This looks like pretty much a straight text subs to me. I can see why you've introduced As ever, thanks very much for your contributions. |
|
ok, I'll review the log changes to avoid silent failures. |
|
Hi @matt335672 , I've modified the log messages in error code paths to use Since we are discussing log messages, I also want to mention that I've made pull request #1742 which adds on top of this pull request more detailed and clear logging messages in the libxrdp code. I created it as a separate pull request to separate the text substitution from improving the log messages. |
libxrdp/xrdp_caps.c
Outdated
| self->client_info.jpeg_codec_id = codec_id; | ||
| i1 = MIN(64, codec_properties_length); | ||
| g_memcpy(self->client_info.jpeg_prop, s->p, i1); | ||
| self->client_info.jpeg_prop_len = i1; | ||
| /* make sure that requested quality is between 0 to 100 */ | ||
| if (self->client_info.jpeg_prop[0] < 0 || self->client_info.jpeg_prop[0] > 100) | ||
| { | ||
| g_writeln(" Warning: the requested jpeg quality (%d) is invalid," | ||
| LOG_DEVEL(LOG_LEVEL_WARNING, " Warning: the requested jpeg quality (%d) is invalid," |
libxrdp/xrdp_rdp.c
Outdated
| LOG(LOG_LEVEL_ERROR, "out xrdp_rdp_recv error"); | ||
| LOG(LOG_LEVEL_ERROR, "xrdp_rdp_recv: xrdp_sec_recv failed"); |
There was a problem hiding this comment.
Probably can remove one of these
libxrdp/xrdp_sec.c
Outdated
| LOG(LOG_LEVEL_ERROR, "xrdp_sec_process_logon_info: flags wrong, major error"); | ||
| LOG(LOG_LEVEL_ERROR, "xrdp_sec_process_logon_info: flags wrong, likely decrypt " |
There was a problem hiding this comment.
Probably can remove one of these
libxrdp/xrdp_sec.c
Outdated
| LOG(LOG_LEVEL_ERROR, " out xrdp_sec_recv : error"); | ||
| LOG(LOG_LEVEL_ERROR, "xrdp_sec_recv: xrdp_mcs_recv failed"); |
There was a problem hiding this comment.
Probably can remove one of these
|
Really good stuff BTW - I can see where some real thought has gone in to some of these. @metalefty - given this is general substitution (plus of course the re-formatting), I'm happy to merge this. Let me know what you think. |
|
I've implemented @matt335672 recommendations, and I think this is ready to merge. Please let me know if there is anything else that needs to be cleaned up or squashed before merging. |
| @@ -135,18 +131,18 @@ libxrdp_force_read(struct trans* trans) | |||
|
|
|||
| if (trans_force_read(trans, 4) != 0) | |||
| { | |||
| g_writeln("libxrdp_force_read: header read error"); | |||
| LOG(LOG_LEVEL_WARNING, "libxrdp_force_read: header read error"); | |||
There was a problem hiding this comment.
Thank you very much for your careful work! You did this not just by replacing them with a single log level but considered the log context.
| { | ||
| /* certificate or privkey is not readable */ | ||
| log_message(LOG_LEVEL_DEBUG, "No readable certificates or " | ||
| "private keys, cannot accept TLS connections"); | ||
| LOG(LOG_LEVEL_WARNING, "No readable certificates or " |
|
Let's merge. |
Follow-on pull request to #1633 which migrates all logging in the libxrdp directory to use the LOG() and LOG_DEVEL() macros.