Re: macaddr 64 bit (EUI-64) datatype support

On Fri, Jan 6, 2017 at 3:51 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
wrote:

> 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:
>
>
Thanks for the review.


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

Updated the documentation to point to correct 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.
>

Fixed.


> 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.
>

Changed accordingly.


> 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.
>

Ok. Removed the above code that blocks the input.


>
> 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.
>

Updated as per you suggestion.


> 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".
>

Added the hint message to explain in detail about addresses that are
eligible for
conversion from macaddr8 type 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").
>

Done the indentation to correct the problem.


> 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



Corrected.

Updated patch is attached.

Regards,
Hari Babu
Fujitsu Australia