On Mon, Jan 9, 2017 at 1:45 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote: > Updated patch is attached. > I've a few comments about the patch.
Thanks for the review.
+ This type can accept both 6 and 8 bytes length MAC addresses. A 6 bytes length MAC address is internally converted to 8 bytes. We should include this in the docs. Because output is always 8 bytes. Otherwise, a user may be surprised with the output.
updated accordingly.
+#define hibits(addr) \ + ((unsigned long)(((addr)->a<<24)|((addr)->b<<16)|((addr)->c<<8)|((addr)->d))) + +#define lobits(addr) \ + ((unsigned long)(((addr)->e<<24)|((addr)->f<<16)|((addr)->g<<8)|((addr)->h))) + There should be some spacing.
corrected.
+ if (!eight_byte_address) + { + d = 0xFF; + e = 0xFE; + } You already have count variable. Just check (count != 8).
Changed.
+ res *= 256 * 256; + res *= 256 * 256; Bit shift operator can be used for this. For example: (res << 32).
Changed by adding a typecast, because res is a double datatype value.
-DATA(insert ( 403 macaddr_ops PGNSP PGUID 1984 829 t 0 )); -DATA(insert ( 405 macaddr_ops PGNSP PGUID 1985 829 t 0 )); +DATA(insert ( 403 macaddr_ops PGNSP PGUID 1984 829 t 0 )); +DATA(insert ( 405 macaddr_ops PGNSP PGUID 1985 829 t 0 )); This is unnecessary I guess.
Corrected.
There was some whitespace error while applying the patch. Also, there are some spacing inconsistencies in the comments. I've not tested with this patch. I'll let you know once I'm done testing.
