Re: [HACKERS] macaddr 64 bit (EUI-64) datatype support

On 1/4/17, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> On Tue, Nov 29, 2016 at 8:36 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>> Updated patch attached with added cast function from macaddr8 to
>> macaddr.
>>
>> Currently there are no support for cross operators. Is this required
>> to be this patch only or can be handled later if required?
>>
>
> Updated patch attached to address the duplicate OID problem.
> There are no other functional changes to the previous patch.

Hello,

several thoughts about the patch:


Documentation:
1.
+     The remaining six input formats are not part of any standard.
Which ones (remaining six formats)?


2.
+ <function>trunc(<type>macaddr8</type>)</function></literal> returns a MAC
+   address with the last 3 bytes set to zero.
It is a misprinting or a copy-paste error.
The implementation and the standard says that the last five bytes are
set to zero and the first three are left as is.


3.
+ for lexicographical ordering
I'm not a native English speaker, but I'd say just "for ordering"
since there are no words, it is just a big number with a special
input/output format.


The code:
4.
+    if ((a == 0) && (b == 0) && (c == 0) && (d == 0)
+            && (e == 0) && (f == 0) && (g == 0) && (h == 0))
...
+    if ((a == 255) && (b == 255) && (c == 255) && (d == 255)
+            && (e == 255) && (f == 255) && (g == 255) && (h == 255))
The standard forbids these values from using in real hardware, no
reason to block them as input data.
Moreover these values can be stored as a result of binary operations,
users can dump them but can not restore.


5.
+    if (!eight_byte_address)
+    {
+        result->d = 0xFF;
...

Don't you think the next version is simplier (all sscanf for macaddr6
skip "d" and "e")?

+    count = sscanf(str, "%x:%x:%x:%x:%x:%x%1s",
+                   &a, &b, &c, &f, &g, &h, junk);
...
+    if (!eight_byte_address)
+    {
+        d = 0xFF;
+        e = 0xFE;
+    }
+    result->a = a;
+    result->b = b;
+    result->c = c;
+    result->d = d;
+    result->e = e;
+    result->f = f;
+    result->g = g;
+    result->h = h;

Also:
+    if (buf->len == 6)
+    {
+        addr->d = 0xFF;
+        addr->e = 0xFE;
+        addr->f = pq_getmsgbyte(buf);
+        addr->g = pq_getmsgbyte(buf);
+        addr->h = pq_getmsgbyte(buf);
+    }
+    else
+    {
+        addr->d = pq_getmsgbyte(buf);
+        addr->e = pq_getmsgbyte(buf);
+        addr->f = pq_getmsgbyte(buf);
+        addr->g = pq_getmsgbyte(buf);
+        addr->h = pq_getmsgbyte(buf);
+    }

can be written as:
+    if (buf->len == 6)
+    {
+        addr->d = 0xFF;
+        addr->e = 0xFE;
+    }
+    else
+    {
+        addr->d = pq_getmsgbyte(buf);
+        addr->e = pq_getmsgbyte(buf);
+    }
+    addr->f = pq_getmsgbyte(buf);
+    addr->g = pq_getmsgbyte(buf);
+    addr->h = pq_getmsgbyte(buf);

but it is only my point of view (don't need to pay close attention
that only those two bytes are written differently, not the last tree
ones), it is not an error.


6.
+                 errmsg("macaddr8 out of range to convert to macaddr")));
I think a hint should be added which values are allowed to convert to "macaddr".

7.
+DATA(insert (  829    774    4123 i f ));
+DATA(insert (  774  829       4124 i f ));
It is a nitpicking, but try to use tabs as in the code around.
(tab between "774" and "829" and space instead of tab between "829" and "4124").

8.
+#define hibits(addr) \
+  ((unsigned long)(((addr)->a<<24)|((addr)->b<<16)|((addr)->c<<8)))
+
+#define lobits(addr) \
+  ((unsigned long)(((addr)->d<<16)|((addr)->e<<8)|((addr)->f)))
+
+#define lobits_extra(addr) \
+  ((unsigned long)((addr)->g<<8)|((addr)->h))

I mentioned that fitting all 4 bytes is a wrong idea[1]
> The macros "hibits" should be 3 octets long, not 4;

... but now I do not think so (there is no UB[2] because source and
destination are not signed).
Moreover you've already fill in "hibits" the topmost byte by shifting by 24.
If you use those two macros ("hibits" and "lobits") it allows to avoid
two extra comparisons in macaddr8_cmp_internal.
Version from the "macaddr64_poc.patch" is correct.


[1]https://www.postgresql.org/message-id/CAKOSWNng9_+=FVO6OZ4TGv1KKHmoM11anKihBoKPuZki1cAkLQ@mail.gmail.com
[2]http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1817.htm


-- 
Best regards,
Vitaly Burovoy