Re: [PATCH net-next 2/2] selftests: net: rds: add getsockopt() conversion test

[Breno Leitao <leitao@debian.org>]

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