GNOME Bugzilla – Bug 777641
pcapparse: add support of parsing by mac addresses and ethertype for raw ethernet frames.
Last modified: 2018-11-03 14:03:58 UTC
Created attachment 344021 [details] [review] patch Add support for parsing pcap files for some specific protocol types. This is required, for example, to parse and replay IEEE1722 streams.
Hi, what is the status of reviewing this patch? Thanks, Konstantin.
Review of attachment 344021 [details] [review]: These lines need to be removed: unnecessary style changes -static gboolean gst_pcap_sink_event (GstPad * pad, - GstObject * parent, GstEvent * event); +static gboolean gst_pcap_sink_event (GstPad * pad, GstObject * parent, + GstEvent * event); guint should not be set to "-1": unnecessary code complexity + self->ethertype = -1; Use larger int type if you need negative values.
Review of attachment 344021 [details] [review]: Please follow the guidelines on https://gstreamer.freedesktop.org/documentation/contribute/#how-to-prepare-a-patch-for-submission
Review of attachment 344021 [details] [review]: ::: gst/pcapparse/gstpcapparse.c @@ +48,3 @@ #include <string.h> +#include <glib-2.0/gobject/gparamspecs.h> +#include <glib-2.0/gobject/gvaluetypes.h> Including these is not necessary @@ +55,3 @@ #include <netinet/in.h> #include <string.h> +#include <gstreamer-1.0/gst/gstinfo.h> and this neither. @@ +145,3 @@ g_param_spec_boxed ("caps", "Caps", + "The caps of the source pad", + GST_TYPE_CAPS, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); Please don't change the indentation of unrelated code @@ +378,3 @@ + { + const gchar *src_mac_str = g_value_get_string (value); + if (parse_mac_to_uint (src_mac_str, self->src_mac) < 0) { src_mac_str might be NULL but your parsing function does not handle that @@ +467,3 @@ + vlan = 1; + eth_type = GUINT16_FROM_BE (*((guint16 *) (buf + 16))); + if (eth_type == 0x8100) // VLAN VLAN support was added in https://bugzilla.gnome.org/show_bug.cgi?id=785778 already, independent of this @@ +483,3 @@ + || (self->src_mac[3] != (*((guint8 *) (buf + 9)))) + || (self->src_mac[4] != (*((guint8 *) (buf + 10)))) + || (self->src_mac[5] != (*((guint8 *) (buf + 11))))) You could use memcmp() here @@ +554,3 @@ + *payload_size = len - UDP_HEADER_LEN; + } else { + if (buf_proto + 12 >= buf + buf_size) For everything but 0x800, we don't parse anything out and just pass onwards any e.g. IP and UDP header? That seems suboptimal :) How would this be used? @@ +733,3 @@ + segment.start = self->offset; + } else { + segment.start = self->base_ts; Why?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/511.