[Bug 777641] pcapparse: add support of parsing by mac addresses and ethertype for raw ethernet frames.

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=777641

--- Comment #4 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 344021:
 --> (https://bugzilla.gnome.org/review?bug=777641&attachment=344021)

::: 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?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.