On Fri, Jun 05, 2026 at 02:56:09AM -0700, Allison Henderson wrote:
> Hi Breno,
>
> Thanks for working on this. Your test covers a lot of socket behavior that
> the
> existing packet test doesn't get into, so I definitely think it's worth
> adding.
> I ran it locally under vng and it passes for me. I did notice one nit below,
> but
> otherwise I think it looks really good.
Good, thanks for the direction and review!
> > +TEST_F(rds, info_counters_snapshot)
> > +{
> > + struct rds_info_counter *ctr;
> > + socklen_t need = 0, len;
> > + long pagesz = sysconf(_SC_PAGESIZE);
> > + unsigned int i, n;
> > + char *region, *buf;
> > + int ret;
> > +
> > + /* Probe for the required size. */
> > + getsockopt(self->fd, SOL_RDS, RDS_INFO_COUNTERS, NULL, &need);
> > + ASSERT_GT(need, 0);
> > +
> > + region = mmap(NULL, 2 * pagesz, PROT_READ | PROT_WRITE,
> > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>
> The 2-page mapping (and the ASSERT_LE guarding it) ties the test to the
> snapshot
> fitting in two pages. This works right now, but ideally the region here
> should
> account for the number of counters that come back from RDS_INFO_COUNTERS so
> that
> if the total number of counters were to grow later, we don't overrun a region
> that's set to a fixed number of pages. So in this case, we'd want the need +
> offset,
> and then page align that so we can mmap it:
>
> offset = pagesz - 64;
> map_len = ((offset + need + pagesz - 1) / pagesz) * pagesz; /* round
> (off+need) up to whole pages */
> region = mmap(NULL, map_len, ...);
Good point - sizing the mapping from the probed length keeps it correct if the
counter set ever grows. In v2 I'll derive map_len from need + offset and drop
the fixed 2-page assumption:
long pagesz = sysconf(_SC_PAGESIZE);
size_t offset, map_len;
...
getsockopt(self->fd, SOL_RDS, RDS_INFO_COUNTERS, NULL, &need);
ASSERT_GT(need, 0);
/* Unaligned start that runs past the first page boundary. */
offset = pagesz - 64;
map_len = ((offset + need + pagesz - 1) / pagesz) * pagesz;
region = mmap(NULL, map_len, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ASSERT_NE(MAP_FAILED, region);
buf = region + offset;
with the ASSERT_LE() dropped and munmap(region, map_len) at the end.
Thanks for the review,
--breno
