After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 727746 - Factor buffer flags-to-string code
Factor buffer flags-to-string code
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 1.3.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-04-07 13:58 UTC by Vincent Penquerc'h
Modified: 2014-06-04 11:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
change fixed size buffer usage (1.76 KB, patch)
2014-04-07 13:58 UTC, Vincent Penquerc'h
committed Details | Review
cleanup (904 bytes, patch)
2014-04-07 13:59 UTC, Vincent Penquerc'h
committed Details | Review
factor two other instances (7.14 KB, patch)
2014-04-07 13:59 UTC, Vincent Penquerc'h
reviewed Details | Review
factored and private (10.42 KB, patch)
2014-06-03 15:09 UTC, Vincent Penquerc'h
needs-work Details | Review
factored and pivate (10.19 KB, patch)
2014-06-04 09:59 UTC, Vincent Penquerc'h
committed Details | Review

Description Vincent Penquerc'h 2014-04-07 13:58:06 UTC
While fixing a strcpy-to-fixed-size-buffer issue found by Coverity, I saw the code that had this was present in three different files. I factored this in gstbuffer.c, but this may warrant comments about the API addition so I'll put it here first. First two patches are what I'd also do to the other two instances of that code should the third patch be NAKed.
Comment 1 Vincent Penquerc'h 2014-04-07 13:58:53 UTC
Created attachment 273710 [details] [review]
change fixed size buffer usage
Comment 2 Vincent Penquerc'h 2014-04-07 13:59:18 UTC
Created attachment 273711 [details] [review]
cleanup
Comment 3 Vincent Penquerc'h 2014-04-07 13:59:45 UTC
Created attachment 273712 [details] [review]
factor two other instances
Comment 4 Sebastian Dröge (slomo) 2014-04-08 08:31:06 UTC
Comment on attachment 273712 [details] [review]
factor two other instances

"Since: 1.4", not 1.3.1 :)

I think I would prefer to have that in a private header in plugins/elements somewhere instead of public API.
Comment 5 Tim-Philipp Müller 2014-04-08 08:43:54 UTC
Also a bit iffy because there are more buffer flags one might want to show, but they're media type specific and defined in -base for example.
Comment 6 Vincent Penquerc'h 2014-04-08 10:03:54 UTC
Would it be less iffy if I added an optional array to the function that maps later flags to custom names ?
Comment 7 Vincent Penquerc'h 2014-04-17 08:09:28 UTC
Tim, would this change in comment 6 assuage your concern ?
Comment 8 Tim-Philipp Müller 2014-04-17 08:22:27 UTC
Not really, I think that's even more ugly :)

I can live with patch #3, the first two are obsolete then I guess.
Comment 9 Vincent Penquerc'h 2014-06-02 11:34:50 UTC
Hmm, I'm not sure what's the consensus here. Tim, you agreed with the third patch, but the first two were marked as commit-now (not sure by whom). If I was to push the third patch, the only comment about it is to move it to a private header ?
Comment 10 Nicolas Dufresne (ndufresne) 2014-06-02 13:16:16 UTC
(In reply to comment #9)
> patch, but the first two were marked as commit-now (not sure by whom).

Sebastian marked it.
Comment 11 Tim-Philipp Müller 2014-06-02 22:52:22 UTC
(In reply to comment #9)
> Hmm, I'm not sure what's the consensus here. Tim, you agreed with the third
> patch, but the first two were marked as commit-now (not sure by whom). If I was
> to push the third patch, the only comment about it is to move it to a private
> header ?

The two first patches are independent afaict and should be pushed.

The code duplication makes sense to de-duplicate. Question was how to best do that. The two options afaict are the third patch or a variant thereof as suggested in comment #6. I would prefer to de-duplicate the code but not make it public API for the time being, and if I interpret Sebastian's comment #4 correctly he also prefers that.
Comment 12 Vincent Penquerc'h 2014-06-03 15:09:54 UTC
Created attachment 277806 [details] [review]
factored and private

Now private to elements.
Comment 13 Sebastian Dröge (slomo) 2014-06-04 09:30:15 UTC
Review of attachment 277806 [details] [review]:

::: plugins/elements/gstelements_private.c
@@ +24,3 @@
+#  include "config.h"
+# endif
+#endif

And why here this #ifndef GST_LICENSE foo? Seems like something wrong with the includes somewhere

@@ +67,3 @@
+      end++;
+    }
+  }

I would prefer if this just used GString but otherwise ok :)

::: plugins/elements/gstelements_private.h
@@ +28,3 @@
+#  include "config.h"
+# endif
+#endif

Why this? Just include config.h in the source file
Comment 14 Vincent Penquerc'h 2014-06-04 09:59:31 UTC
Created attachment 277862 [details] [review]
factored and pivate

This include setup was from gst_private.[ch]. Works fine with the GST_LICENCE bit taken out.

Do you really want GString use ? I don't really see the point of adding some extra API use for something so simple.
Comment 15 Sebastian Dröge (slomo) 2014-06-04 10:03:33 UTC
Comment on attachment 277862 [details] [review]
factored and pivate

I'm fine with not using GString here.

But arguably that code is not really simple and has quite some potential for causing crashes and unexpected results if written badly :) I mean you also wouldn't write that in assembler, would you? GString just makes it much easier and keeps all the ugly work out of your way
Comment 16 Vincent Penquerc'h 2014-06-04 10:12:08 UTC
> wouldn't write that in assembler, would you? 

I dunno about that... Brings back some happy hacking memories :P

But OK, it's not so simple, granted.
Comment 17 Vincent Penquerc'h 2014-06-04 11:36:26 UTC
Thanks. Pushed.

commit a8ca56b42d785e4deaa4d1aa141d4734f8693a66
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Mon Apr 7 14:49:59 2014 +0100

    gstbuffer: factor three flags-to-string loops