Re: bgpd use long long instead of int64_t
On Tue, Feb 19, 2019 at 09:28:36AM +0100, Claudio Jeker wrote:
> On Tue, Feb 19, 2019 at 12:10:25AM +0100, Andreas Kusalananda Kähäri wrote:
> > On Mon, Feb 18, 2019 at 10:11:03PM +0100, Mark Kettenis wrote:
> > > > Date: Mon, 18 Feb 2019 21:59:38 +0100
> > > > From: Claudio Jeker
> > > >
> > > > In some places bgpd just wants something bigger then a 32bit int.
> > > > Instead of using int64_t or u_int64_t use (unsigned) long long which is
> > > > at
> > > > least 64bit and therefor good enough. Makes the mess with type
> > > > definition
> > > > of int64_t on various systems go away (including a bunch of type casts).
> > > > While there also apply the endian.h cleanup done in bgpd a few days ago.
> > > >
> > > > OK?
> > >
> > > You could use and uint64_t instead. That should be
> > > portable. But you'd still need to be careful about printf statements
> > > since (u)int64_t might be (unsigned) long on some systems.
> >
> > printf should be no issue if you use the correct PRI*64 (PRIu64 or
> > PRId64) macro from . Both and are
> > C99.
> >
> > E.g.
> >
> > uint64_t thing;
> >
> > /* ... */
> >
> > printf("The value is %" PRIu64 "\n", thing);
> >
> >
> > ... but I'm really not qualified to say anything about what you guys should
> > do.
> >
>
> While true I have to say that the PRI constructs are even worse than doing
> casts. Chopping up the format string like this is just ugly and unreadable
> for complex format strings.
Yes, and the only other alternative I can think of: (u)intmax_t and
%jd or %ju is not very attractive either.
-Otto
Re: bgpd use long long instead of int64_t
On Tue, Feb 19, 2019 at 12:10:25AM +0100, Andreas Kusalananda Kähäri wrote:
> On Mon, Feb 18, 2019 at 10:11:03PM +0100, Mark Kettenis wrote:
> > > Date: Mon, 18 Feb 2019 21:59:38 +0100
> > > From: Claudio Jeker
> > >
> > > In some places bgpd just wants something bigger then a 32bit int.
> > > Instead of using int64_t or u_int64_t use (unsigned) long long which is at
> > > least 64bit and therefor good enough. Makes the mess with type definition
> > > of int64_t on various systems go away (including a bunch of type casts).
> > > While there also apply the endian.h cleanup done in bgpd a few days ago.
> > >
> > > OK?
> >
> > You could use and uint64_t instead. That should be
> > portable. But you'd still need to be careful about printf statements
> > since (u)int64_t might be (unsigned) long on some systems.
>
> printf should be no issue if you use the correct PRI*64 (PRIu64 or
> PRId64) macro from . Both and are
> C99.
>
> E.g.
>
> uint64_t thing;
>
> /* ... */
>
> printf("The value is %" PRIu64 "\n", thing);
>
>
> ... but I'm really not qualified to say anything about what you guys should
> do.
>
While true I have to say that the PRI constructs are even worse than doing
casts. Chopping up the format string like this is just ugly and unreadable
for complex format strings.
--
:wq Claudio
Re: bgpd use long long instead of int64_t
On Mon, Feb 18, 2019 at 10:11:03PM +0100, Mark Kettenis wrote:
> > Date: Mon, 18 Feb 2019 21:59:38 +0100
> > From: Claudio Jeker
> >
> > In some places bgpd just wants something bigger then a 32bit int.
> > Instead of using int64_t or u_int64_t use (unsigned) long long which is at
> > least 64bit and therefor good enough. Makes the mess with type definition
> > of int64_t on various systems go away (including a bunch of type casts).
> > While there also apply the endian.h cleanup done in bgpd a few days ago.
> >
> > OK?
>
> You could use and uint64_t instead. That should be
> portable. But you'd still need to be careful about printf statements
> since (u)int64_t might be (unsigned) long on some systems.
printf should be no issue if you use the correct PRI*64 (PRIu64 or
PRId64) macro from . Both and are
C99.
E.g.
uint64_t thing;
/* ... */
printf("The value is %" PRIu64 "\n", thing);
... but I'm really not qualified to say anything about what you guys should do.
Cheers,
>
> Oh, long long doesn't work on MSVC, but you probably don't care about
> that.
>
> > --
> > :wq Claudio
> >
> > Index: usr.sbin/bgpctl/bgpctl.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> > retrieving revision 1.229
> > diff -u -p -r1.229 bgpctl.c
> > --- usr.sbin/bgpctl/bgpctl.c11 Feb 2019 15:47:55 - 1.229
> > +++ usr.sbin/bgpctl/bgpctl.c18 Feb 2019 20:53:38 -
> > @@ -23,6 +23,7 @@
> > #include
> > #include
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -84,7 +85,6 @@ void show_attr(void *, u_int16_t, int)
> > voidshow_community(u_char *, u_int16_t);
> > voidshow_large_community(u_char *, u_int16_t);
> > voidshow_ext_community(u_char *, u_int16_t);
> > -char *fmt_mem(int64_t);
> > int show_rib_memory_msg(struct imsg *);
> > voidsend_filterset(struct imsgbuf *, struct
> > filter_set_head *);
> > const char *get_errstr(u_int8_t, u_int8_t);
> > @@ -1761,12 +1761,12 @@ show_ext_community(u_char *data, u_int16
> > case EXT_COMMUNITY_TRANS_OPAQUE:
> > case EXT_COMMUNITY_TRANS_EVPN:
> > memcpy(, data + i, sizeof(ext));
> > - ext = betoh64(ext) & 0xLL;
> > - printf("0x%llx", ext);
> > + ext = be64toh(ext) & 0xLL;
> > + printf("0x%llx", (unsigned long long)ext);
> > break;
> > case EXT_COMMUNITY_NON_TRANS_OPAQUE:
> > memcpy(, data + i, sizeof(ext));
> > - ext = betoh64(ext) & 0xLL;
> > + ext = be64toh(ext) & 0xLL;
> > switch (ext) {
> > case EXT_COMMUNITY_OVS_VALID:
> > printf("valid ");
> > @@ -1778,26 +1778,26 @@ show_ext_community(u_char *data, u_int16
> > printf("invalid ");
> > break;
> > default:
> > - printf("0x%llx ", ext);
> > + printf("0x%llx ", (unsigned long long)ext);
> > break;
> > }
> > break;
> > default:
> > memcpy(, data + i, sizeof(ext));
> > - printf("0x%llx", betoh64(ext));
> > + printf("0x%llx", (unsigned long long)be64toh(ext));
> > }
> > if (i + 8 < len)
> > printf(", ");
> > }
> > }
> >
> > -char *
> > -fmt_mem(int64_t num)
> > +static char *
> > +fmt_mem(long long num)
> > {
> > static char buf[16];
> >
> > if (fmt_scaled(num, buf) == -1)
> > - snprintf(buf, sizeof(buf), "%lldB", (long long)num);
> > + snprintf(buf, sizeof(buf), "%lldB", num);
> >
> > return (buf);
> > }
> > @@ -1822,31 +1822,31 @@ show_rib_memory_msg(struct imsg *imsg)
> > continue;
> > pts += stats.pt_cnt[i] * pt_sizes[i];
> > printf("%10lld %s network entries using %s of memory\n",
> > - (long long)stats.pt_cnt[i], aid_vals[i].name,
> > + stats.pt_cnt[i], aid_vals[i].name,
> > fmt_mem(stats.pt_cnt[i] * pt_sizes[i]));
> > }
> > printf("%10lld rib entries using %s of memory\n",
> > - (long long)stats.rib_cnt, fmt_mem(stats.rib_cnt *
> > + stats.rib_cnt, fmt_mem(stats.rib_cnt *
> > sizeof(struct rib_entry)));
> > printf("%10lld prefix entries using %s of memory\n",
> > - (long long)stats.prefix_cnt, fmt_mem(stats.prefix_cnt *
> > + stats.prefix_cnt,
Re: bgpd use long long instead of int64_t
On Mon, Feb 18, 2019 at 02:24:52PM -0700, Theo de Raadt wrote: > Claudio Jeker wrote: > > > On Mon, Feb 18, 2019 at 10:11:03PM +0100, Mark Kettenis wrote: > > > > Date: Mon, 18 Feb 2019 21:59:38 +0100 > > > > From: Claudio Jeker > > > > > > > > In some places bgpd just wants something bigger then a 32bit int. > > > > Instead of using int64_t or u_int64_t use (unsigned) long long which is > > > > at > > > > least 64bit and therefor good enough. Makes the mess with type > > > > definition > > > > of int64_t on various systems go away (including a bunch of type casts). > > > > While there also apply the endian.h cleanup done in bgpd a few days ago. > > > > > > > > OK? > > > > > > You could use and uint64_t instead. That should be > > > portable. But you'd still need to be careful about printf statements > > > since (u)int64_t might be (unsigned) long on some systems. > > > > This issue with int64_t being just a unsigned long on 64bit > > Huh? Surely you mispoke, it is not unsigned. Arrrgg. int64_t -> long, u_int64_t aka uint64_t -> unsigned long Had too much type handling today. > linux is the > > problem I'm trying to avoid since the result is that all printf calls need > > casts. long long is %llu on all systems I care and so less ugly in this > > specific case. > > Huh? Again I'm confused. long long is %lld, not %llu Yes, that should have been %lld. Should have payed more attention while answering. -- :wq Claudio
Re: bgpd use long long instead of int64_t
Claudio Jeker wrote: > On Mon, Feb 18, 2019 at 10:11:03PM +0100, Mark Kettenis wrote: > > > Date: Mon, 18 Feb 2019 21:59:38 +0100 > > > From: Claudio Jeker > > > > > > In some places bgpd just wants something bigger then a 32bit int. > > > Instead of using int64_t or u_int64_t use (unsigned) long long which is at > > > least 64bit and therefor good enough. Makes the mess with type definition > > > of int64_t on various systems go away (including a bunch of type casts). > > > While there also apply the endian.h cleanup done in bgpd a few days ago. > > > > > > OK? > > > > You could use and uint64_t instead. That should be > > portable. But you'd still need to be careful about printf statements > > since (u)int64_t might be (unsigned) long on some systems. > > This issue with int64_t being just a unsigned long on 64bit Huh? Surely you mispoke, it is not unsigned. linux is the > problem I'm trying to avoid since the result is that all printf calls need > casts. long long is %llu on all systems I care and so less ugly in this > specific case. Huh? Again I'm confused. long long is %lld, not %llu
Re: bgpd use long long instead of int64_t
On Mon, Feb 18, 2019 at 10:11:03PM +0100, Mark Kettenis wrote:
> > Date: Mon, 18 Feb 2019 21:59:38 +0100
> > From: Claudio Jeker
> >
> > In some places bgpd just wants something bigger then a 32bit int.
> > Instead of using int64_t or u_int64_t use (unsigned) long long which is at
> > least 64bit and therefor good enough. Makes the mess with type definition
> > of int64_t on various systems go away (including a bunch of type casts).
> > While there also apply the endian.h cleanup done in bgpd a few days ago.
> >
> > OK?
>
> You could use and uint64_t instead. That should be
> portable. But you'd still need to be careful about printf statements
> since (u)int64_t might be (unsigned) long on some systems.
This issue with int64_t being just a unsigned long on 64bit linux is the
problem I'm trying to avoid since the result is that all printf calls need
casts. long long is %llu on all systems I care and so less ugly in this
specific case. I used uint64_t mostly because it is shorter than
unsigned long long (being lazy is not always a win).
> Oh, long long doesn't work on MSVC, but you probably don't care about
> that.
Nope, just BSD and linux (maybe other UNIX variants).
> > --
> > :wq Claudio
> >
> > Index: usr.sbin/bgpctl/bgpctl.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> > retrieving revision 1.229
> > diff -u -p -r1.229 bgpctl.c
> > --- usr.sbin/bgpctl/bgpctl.c11 Feb 2019 15:47:55 - 1.229
> > +++ usr.sbin/bgpctl/bgpctl.c18 Feb 2019 20:53:38 -
> > @@ -23,6 +23,7 @@
> > #include
> > #include
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -84,7 +85,6 @@ void show_attr(void *, u_int16_t, int)
> > voidshow_community(u_char *, u_int16_t);
> > voidshow_large_community(u_char *, u_int16_t);
> > voidshow_ext_community(u_char *, u_int16_t);
> > -char *fmt_mem(int64_t);
> > int show_rib_memory_msg(struct imsg *);
> > voidsend_filterset(struct imsgbuf *, struct
> > filter_set_head *);
> > const char *get_errstr(u_int8_t, u_int8_t);
> > @@ -1761,12 +1761,12 @@ show_ext_community(u_char *data, u_int16
> > case EXT_COMMUNITY_TRANS_OPAQUE:
> > case EXT_COMMUNITY_TRANS_EVPN:
> > memcpy(, data + i, sizeof(ext));
> > - ext = betoh64(ext) & 0xLL;
> > - printf("0x%llx", ext);
> > + ext = be64toh(ext) & 0xLL;
> > + printf("0x%llx", (unsigned long long)ext);
> > break;
> > case EXT_COMMUNITY_NON_TRANS_OPAQUE:
> > memcpy(, data + i, sizeof(ext));
> > - ext = betoh64(ext) & 0xLL;
> > + ext = be64toh(ext) & 0xLL;
> > switch (ext) {
> > case EXT_COMMUNITY_OVS_VALID:
> > printf("valid ");
> > @@ -1778,26 +1778,26 @@ show_ext_community(u_char *data, u_int16
> > printf("invalid ");
> > break;
> > default:
> > - printf("0x%llx ", ext);
> > + printf("0x%llx ", (unsigned long long)ext);
> > break;
> > }
> > break;
> > default:
> > memcpy(, data + i, sizeof(ext));
> > - printf("0x%llx", betoh64(ext));
> > + printf("0x%llx", (unsigned long long)be64toh(ext));
> > }
> > if (i + 8 < len)
> > printf(", ");
> > }
> > }
> >
> > -char *
> > -fmt_mem(int64_t num)
> > +static char *
> > +fmt_mem(long long num)
> > {
> > static char buf[16];
> >
> > if (fmt_scaled(num, buf) == -1)
> > - snprintf(buf, sizeof(buf), "%lldB", (long long)num);
> > + snprintf(buf, sizeof(buf), "%lldB", num);
> >
> > return (buf);
> > }
> > @@ -1822,31 +1822,31 @@ show_rib_memory_msg(struct imsg *imsg)
> > continue;
> > pts += stats.pt_cnt[i] * pt_sizes[i];
> > printf("%10lld %s network entries using %s of memory\n",
> > - (long long)stats.pt_cnt[i], aid_vals[i].name,
> > + stats.pt_cnt[i], aid_vals[i].name,
> > fmt_mem(stats.pt_cnt[i] * pt_sizes[i]));
> > }
> > printf("%10lld rib entries using %s of memory\n",
> > - (long long)stats.rib_cnt, fmt_mem(stats.rib_cnt *
> > + stats.rib_cnt, fmt_mem(stats.rib_cnt *
> > sizeof(struct rib_entry)));
> > printf("%10lld prefix entries using %s of memory\n",
> > - (long
Re: bgpd use long long instead of int64_t
Mark Kettenis wrote: > > Date: Mon, 18 Feb 2019 21:59:38 +0100 > > From: Claudio Jeker > > > > In some places bgpd just wants something bigger then a 32bit int. > > Instead of using int64_t or u_int64_t use (unsigned) long long which is at > > least 64bit and therefor good enough. Makes the mess with type definition > > of int64_t on various systems go away (including a bunch of type casts). > > While there also apply the endian.h cleanup done in bgpd a few days ago. > > > > OK? > > You could use and uint64_t instead. That should be > portable. But you'd still need to be careful about printf statements > since (u)int64_t might be (unsigned) long on some systems. Indeed, that is the problem, the difference between systems where the 64-bit types are "long" vs "long long". They are both valid, but there is a conflict. That requires casts to the raw-type in some places, but not others. It can be confusing, and as a result people have a tendency to (cautiously) overcast. But casting when you shouldn't can be dangerous. Especially downsize casts.
Re: bgpd use long long instead of int64_t
> Date: Mon, 18 Feb 2019 21:59:38 +0100
> From: Claudio Jeker
>
> In some places bgpd just wants something bigger then a 32bit int.
> Instead of using int64_t or u_int64_t use (unsigned) long long which is at
> least 64bit and therefor good enough. Makes the mess with type definition
> of int64_t on various systems go away (including a bunch of type casts).
> While there also apply the endian.h cleanup done in bgpd a few days ago.
>
> OK?
You could use and uint64_t instead. That should be
portable. But you'd still need to be careful about printf statements
since (u)int64_t might be (unsigned) long on some systems.
Oh, long long doesn't work on MSVC, but you probably don't care about
that.
> --
> :wq Claudio
>
> Index: usr.sbin/bgpctl/bgpctl.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.229
> diff -u -p -r1.229 bgpctl.c
> --- usr.sbin/bgpctl/bgpctl.c 11 Feb 2019 15:47:55 - 1.229
> +++ usr.sbin/bgpctl/bgpctl.c 18 Feb 2019 20:53:38 -
> @@ -23,6 +23,7 @@
> #include
> #include
>
> +#include
> #include
> #include
> #include
> @@ -84,7 +85,6 @@ void show_attr(void *, u_int16_t, int)
> void show_community(u_char *, u_int16_t);
> void show_large_community(u_char *, u_int16_t);
> void show_ext_community(u_char *, u_int16_t);
> -char *fmt_mem(int64_t);
> int show_rib_memory_msg(struct imsg *);
> void send_filterset(struct imsgbuf *, struct filter_set_head *);
> const char *get_errstr(u_int8_t, u_int8_t);
> @@ -1761,12 +1761,12 @@ show_ext_community(u_char *data, u_int16
> case EXT_COMMUNITY_TRANS_OPAQUE:
> case EXT_COMMUNITY_TRANS_EVPN:
> memcpy(, data + i, sizeof(ext));
> - ext = betoh64(ext) & 0xLL;
> - printf("0x%llx", ext);
> + ext = be64toh(ext) & 0xLL;
> + printf("0x%llx", (unsigned long long)ext);
> break;
> case EXT_COMMUNITY_NON_TRANS_OPAQUE:
> memcpy(, data + i, sizeof(ext));
> - ext = betoh64(ext) & 0xLL;
> + ext = be64toh(ext) & 0xLL;
> switch (ext) {
> case EXT_COMMUNITY_OVS_VALID:
> printf("valid ");
> @@ -1778,26 +1778,26 @@ show_ext_community(u_char *data, u_int16
> printf("invalid ");
> break;
> default:
> - printf("0x%llx ", ext);
> + printf("0x%llx ", (unsigned long long)ext);
> break;
> }
> break;
> default:
> memcpy(, data + i, sizeof(ext));
> - printf("0x%llx", betoh64(ext));
> + printf("0x%llx", (unsigned long long)be64toh(ext));
> }
> if (i + 8 < len)
> printf(", ");
> }
> }
>
> -char *
> -fmt_mem(int64_t num)
> +static char *
> +fmt_mem(long long num)
> {
> static char buf[16];
>
> if (fmt_scaled(num, buf) == -1)
> - snprintf(buf, sizeof(buf), "%lldB", (long long)num);
> + snprintf(buf, sizeof(buf), "%lldB", num);
>
> return (buf);
> }
> @@ -1822,31 +1822,31 @@ show_rib_memory_msg(struct imsg *imsg)
> continue;
> pts += stats.pt_cnt[i] * pt_sizes[i];
> printf("%10lld %s network entries using %s of memory\n",
> - (long long)stats.pt_cnt[i], aid_vals[i].name,
> + stats.pt_cnt[i], aid_vals[i].name,
> fmt_mem(stats.pt_cnt[i] * pt_sizes[i]));
> }
> printf("%10lld rib entries using %s of memory\n",
> - (long long)stats.rib_cnt, fmt_mem(stats.rib_cnt *
> + stats.rib_cnt, fmt_mem(stats.rib_cnt *
> sizeof(struct rib_entry)));
> printf("%10lld prefix entries using %s of memory\n",
> - (long long)stats.prefix_cnt, fmt_mem(stats.prefix_cnt *
> + stats.prefix_cnt, fmt_mem(stats.prefix_cnt *
> sizeof(struct prefix)));
> printf("%10lld BGP path attribute entries using %s of memory\n",
> - (long long)stats.path_cnt, fmt_mem(stats.path_cnt *
> + stats.path_cnt, fmt_mem(stats.path_cnt *
> sizeof(struct rde_aspath)));
> printf("\t and holding %lld references\n",
> - (long long)stats.path_refs);
> + stats.path_refs);
> printf("%10lld BGP AS-PATH attribute
