GNOME Bugzilla – Bug 768530
shmsink: Check return values of functions operating on memory
Last modified: 2017-12-19 22:33:48 UTC
Created attachment 331014 [details] [review] shmsink: Check return values of functions operating on memory Well, checking whether functions succeeded is just nicer than not checking anything.
Created attachment 331015 [details] [review] shmsink: Check return values of functions operating on memory
Created attachment 331017 [details] [review] shmsink: Check return values of functions operating on memory
Review of attachment 331015 [details] [review]: ::: sys/shm/gstshmsink.c @@ +722,3 @@ + if (!gst_memory_map (memory, &map, GST_MAP_WRITE)) { + GST_ELEMENT_ERROR (self, STREAM, FAILED, + (NULL), The first string is the most important, please swap.
Review of attachment 331017 [details] [review]: Same.
Actually, I think this is fine as is. The first is the message that would be shown to a user. It should not contain any technical details really, and it should ideally be translated. In this case "memory" and the fact that it couldn't be "mapped" are low-level details a normal user wouldn't understand, so they should go into the debug string (second string). Leaving the first as (NULL) means the user gets to see some generic translated 'streaming failed' message, which is as good as it can be here I suppose.
Any clear resolution on this? Now patch is marked as needs-work.
It's waiting for further comment from Nicolas, otherwise it will be pushed by someone else at some point.
Well as you like. Why I'm nitpicking is because if a user didn't enable any trace at all, he will only see the first message, as it is set to NULL, it will be a generic message which will mean nothing and couldn't be "googled" to match any existing reported issues. The message itself should indeed be formulated in less technical manner of course. It's a trivial tiny little change. I just think it's bad practice to not fill at least the first string.
When I was preparing the patch I was looking at the patterns in other more mature elements at this is what I have found when message was rare, purely low level technical issue. I'll do whatever you think it's fine, IMO there's a lack of clear coding standards in that matter.
Fixed indentation and left the default translated error, patch is otherwise good.