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

On Wed, Jan 25, 2017 at 6:43 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
wrote:

> On 1/23/17, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> > The patch is split into two parts.
> > 1. Macaddr8 datatype support
> > 2. Contrib module support.
>
> Hello,
>
> I'm sorry for the delay.
> The patch is almost done, but I have two requests since the last review.
>

Thanks for the review.


> 1.
> src/backend/utils/adt/mac8.c:
> +       int                     a,
> +                               b,
> +                               c,
> +                               d = 0,
> +                               e = 0,
> ...
>
> There is no reason to set them as 0. For EUI-48 they will be
> reassigned in the "if (count != 8)" block, for EUI-64 -- in one of
> sscanf.
> They could be set to "d = 0xFF, e = 0xFE," and avoid the "if" block
> mentioned above, but it makes the code be much less readable.
>
> Oh. I see. In the current version it must be assigned because for
> EUI-48 memory can have values <0 or >255 in uninitialized variables.
> It is better to avoid initialization by merging two parts of the code:
> +       if (count != 6)
> +       {
> +               /* May be a 8-byte MAC address */
> ...
> +       if (count != 8)
> +       {
> +               d = 0xFF;
> +               e = 0xFE;
> +       }
>
> to a single one:
> +       if (count == 6)
> +       {
> +               d = 0xFF;
> +               e = 0xFE;
> +       }
> +       else
> +       {
> +               /* May be a 8-byte MAC address */
> ...
>

Changed accordingly.


> 2.
> src/backend/utils/adt/network.c:
> +                               res = (mac->a << 24) | (mac->b << 16) |
> (mac->c << 8) | (mac->d);
> +                               res = (double)((uint64)res << 32);
> +                               res += (mac->e << 24) | (mac->f << 16) |
> (mac->g << 8) | (mac->h);
>
> Khm... I trust that modern compilers can do a lot of optimizations but
> for me it looks terrible because of needless conversions.
> The reason why earlier versions did have two lines "res *= 256 * 256"
> was an integer overflow for four multipliers, but it can be solved by
> defining the first multiplier as a double:
> +                               res = (mac->a << 24) | (mac->b << 16) |
> (mac->c << 8) | (mac->d);
> +                               res *= (double)256 * 256 * 256 * 256;
> +                               res += (mac->e << 24) | (mac->f << 16) |
> (mac->g << 8) | (mac->h);
>
> In this case the left-hand side argument for the "*=" operator is
> computed at the compile time as a single constant.
> The second line can be written as "res *= 256. * 256 * 256 * 256;"
> (pay attention to a dot in the first multiplier), but it is not
> readable at all (and produces the same code).


Corrected as suggested.

Updated patch attached. There is no change in the contrib patch.

Regards,
Hari Babu
Fujitsu Australia