From: | Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> |
---|---|
To: | Vitaly Burovoy <vitaly(dot)burovoy(at)gmail(dot)com> |
Cc: | Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Shay Rojansky <roji(at)roji(dot)org>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Julien Rouhaud <julien(dot)rouhaud(at)dalibo(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: macaddr 64 bit (EUI-64) datatype support |
Date: | 2017-01-25 12:30:56 |
Message-ID: | CAJrrPGfxhFk84SMWvo8q65KncjANN0cZbMq-KDSPe6exjqdwRA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Jan 25, 2017 at 6:43 PM, Vitaly Burovoy <vitaly(dot)burovoy(at)gmail(dot)com>
wrote:
> On 1/23/17, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
mac_eui64_support_8.patch | application/octet-stream | 46.9 KB |
contrib_macaddr8_1.patch | application/octet-stream | 20.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2017-01-25 12:56:32 | Re: multivariate statistics (v19) |
Previous Message | Rahila Syed | 2017-01-25 12:18:54 | Re: Improvements in psql hooks for variables |