On Mon, Jan 12, 2015 at 05:44:20PM +0100, Reyk Floeter wrote:
> On Mon, Jan 12, 2015 at 09:02:50AM -0600, Brent Cook wrote:
> > On Fri, Jan 09, 2015 at 05:45:17PM -0500, Ted Unangst wrote:
> > > On Fri, Jan 09, 2015 at 15:45, Brent Cook wrote:
> > > > From: Brent Cook <[email protected]>
> > > >
> > > > Yeah yeah, a pointer is a pointer (except when it isn't :). I think this
> > > > looks nicer, since idx2peer is really the thing we're allocating to.
> > >
> > > what about the bzero ten lines later?
> > >
> > > I think the sizeof(*idx2peer) idiom is also a little nicer. and you
> > > can change the other reallocarray too.
> > >
> >
> > Good point, might as well fix them all.
> >
> > This updates all of the applicable sizeof(struct blah) references to use
> > sizeof(thing). No binary change on 32 or 64-bit.
> >
> > Index: client.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/ntpd/client.c,v
> > retrieving revision 1.97
> > diff -u -p -u -p -r1.97 client.c
> > --- client.c 9 Jan 2015 23:44:07 -0000 1.97
> > +++ client.c 12 Jan 2015 14:56:01 -0000
> > @@ -49,7 +49,7 @@ set_deadline(struct ntp_peer *p, time_t
> > int
> > client_peer_init(struct ntp_peer *p)
> > {
> > - if ((p->query = calloc(1, sizeof(struct ntp_query))) == NULL)
> > + if ((p->query = calloc(1, sizeof(*(p->query)))) == NULL)
> > fatal("client_peer_init calloc");
> > p->query->fd = -1;
> > p->query->msg.status = MODE_CLIENT | (NTP_VERSION << 3);
> > Index: config.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/ntpd/config.c,v
> > retrieving revision 1.22
> > diff -u -p -u -p -r1.22 config.c
> > --- config.c 10 Jan 2015 13:47:05 -0000 1.22
> > +++ config.c 12 Jan 2015 14:56:01 -0000
> > @@ -42,7 +42,7 @@ host(const char *s, struct ntp_addr **hn
> > struct ntp_addr *h = NULL;
> >
> > if (!strcmp(s, "*"))
> > - if ((h = calloc(1, sizeof(struct ntp_addr))) == NULL)
> > + if ((h = calloc(1, sizeof(*h))) == NULL)
> > fatal(NULL);
> >
>
> What is the benefit of this mechanical change everywhere?
>
> btw., some of these functions are also used in other daemons in our
> tree that and we tried to keep the changes in sync, with minor
> differences, including host_* and common code from control.c and
> parse.y. I think we can sync this mechanical change (and somebody
> will gain many free commits), but it also makes it harder to follow
> relevant fixes.
>
> Reyk
>
It all started a minimal patch, to made sure we call reallocarray for
idx2peer with sizeof the destination type. Then I fixed pfd and the
bzero calls to match on Ted's suggestion.
Then I noticed inconsistency elsewhere and obviously got carried away.
In other projects, I've usually prefered this notation to avoid type
mismatches. But, it makes sense to avoid needless mutation, especially
since in shared and already correct code.
Would this more appropriately-scoped patch be OK?
Index: ntp.c
===================================================================
RCS file: /cvs/src/usr.sbin/ntpd/ntp.c,v
retrieving revision 1.125
diff -u -p -u -p -r1.125 ntp.c
--- ntp.c 9 Jan 2015 07:35:37 -0000 1.125
+++ ntp.c 12 Jan 2015 14:56:01 -0000
@@ -200,7 +200,7 @@ ntp_main(int pipe_prnt[2], int fd_ctl, s
while (ntp_quit == 0) {
if (peer_cnt > idx2peer_elms) {
if ((newp = reallocarray(idx2peer, peer_cnt,
- sizeof(void *))) == NULL) {
+ sizeof(*idx2peer))) == NULL) {
/* panic for now */
log_warn("could not resize idx2peer from %u -> "
"%u entries", idx2peer_elms, peer_cnt);
@@ -213,7 +213,7 @@ ntp_main(int pipe_prnt[2], int fd_ctl, s
new_cnt = PFD_MAX + peer_cnt + listener_cnt + ctl_cnt;
if (new_cnt > pfd_elms) {
if ((newp = reallocarray(pfd, new_cnt,
- sizeof(struct pollfd))) == NULL) {
+ sizeof(*pfd))) == NULL) {
/* panic for now */
log_warn("could not resize pfd from %u -> "
"%u entries", pfd_elms, new_cnt);
@@ -223,8 +223,8 @@ ntp_main(int pipe_prnt[2], int fd_ctl, s
pfd_elms = new_cnt;
}
- bzero(pfd, sizeof(struct pollfd) * pfd_elms);
- bzero(idx2peer, sizeof(void *) * idx2peer_elms);
+ bzero(pfd, sizeof(*pfd) * pfd_elms);
+ bzero(idx2peer, sizeof(*idx2peer) * idx2peer_elms);
nextaction = getmonotime() + 3600;
pfd[PFD_PIPE_MAIN].fd = ibuf_main->fd;
pfd[PFD_PIPE_MAIN].events = POLLIN;
