GNOME Bugzilla – Bug 745144
gstvalue: Make sure GST_FOURCC_ARGS produces printable characters
Last modified: 2015-02-26 06:50:53 UTC
Make sure we only output printable characters with GST_FOURCC_ARGS
Created attachment 297853 [details] [review] gstvalue: Make sure GST_FOURCC_ARGS produces printable characters Some systems will crash if we use non-printable characters in print/debug statements. Make sure that GST_FOURCC_ARGS never does that
Comment on attachment 297853 [details] [review] gstvalue: Make sure GST_FOURCC_ARGS produces printable characters >-#define GST_FOURCC_ARGS(fourcc) \ >- ((gchar) ((fourcc) &0xff)), \ >- ((gchar) (((fourcc)>>8 )&0xff)), \ >- ((gchar) (((fourcc)>>16)&0xff)), \ >- ((gchar) (((fourcc)>>24)&0xff)) >+ >+#define GST_PRINT_CHAR(fourcc, shift) \ >+ g_ascii_isprint((fourcc)>>shift) ? (fourcc)>>shift : '.' >+#define GST_FOURCC_ARGS(fourcc) \ >+ GST_PRINT_CHAR(fourcc, 0), \ >+ GST_PRINT_CHAR(fourcc, 8), \ >+ GST_PRINT_CHAR(fourcc, 16), \ >+ GST_PRINT_CHAR(fourcc, 24) To me, passing the shift argument to GST_PRINT_CHAR seems a bit weird. If you're making this a publically usable define with the name _CHAR in it, it should just take a single char/uchar IMHO. We also seem to be dropping the explicit &0xff which I think would be good to keep.
I was just trying to avoid a double shift/eval on each variable. I don't really need to make the _CHAR macro public, but didn't want to make the GST_FOURCC_ARGS macro humongous
Created attachment 297898 [details] [review] gstvalue: Make sure GST_FOURCC_ARGS produces printable characters Some systems will crash if we use non-printable characters in print/debug statements. Make sure that GST_FOURCC_ARGS never does that
Comment on attachment 297898 [details] [review] gstvalue: Make sure GST_FOURCC_ARGS produces printable characters Looks good to me. I don't think this is a place where we need to worry about the performance impact of bit shifting :) Perhaps the helper macro should still have a __GST_* prefix just to be safe(r)?
Thanks for the review Pushed a version with __GST_PRINT_CHAR instead commit 075def0f9771dd89503808e7d18f0569f7a76690 Author: Edward Hervey <bilboed@bilboed.com> Date: Wed Feb 25 08:26:19 2015 +0100 gstvalue: Make sure GST_FOURCC_ARGS produces printable characters Some systems will crash if we use non-printable characters in print/debug statements. Make sure that GST_FOURCC_ARGS never does that https://bugzilla.gnome.org/show_bug.cgi?id=745144