[PATCH 2/2] options: Replace use of VLAs in C++

[Laurent Pinchart]

On Thu, Feb 01, 2024 at 09:48:06PM +0000, Barnabás Pőcze wrote:
> 2024. február 1., csütörtök 6:08 keltezéssel, Khem Raj írta:
> 
> Clang++ 18 is fussy about this with new warning checks.
> > 
> >    ../git/src/apps/common/options.cpp:882:20: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
> >       882 |         char shortOptions[optionsMap_.size() * 3 + 2];
> >           |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Therefore replace using VLAs with alloca and malloc/free
> > 
> > Signed-off-by: Khem Raj <raj.khem at gmail.com>
> > ---
> >  src/apps/common/options.cpp      |  4 ++--
> >  src/libcamera/ipc_unixsocket.cpp | 12 ++++++++----
> >  2 files changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/apps/common/options.cpp b/src/apps/common/options.cpp
> > index 4f7e8691..b020f603 100644
> > --- a/src/apps/common/options.cpp
> > +++ b/src/apps/common/options.cpp
> > @@ -879,8 +879,8 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)
> >  	 * Allocate short and long options arrays large enough to contain all
> >  	 * options.
> >  	 */
> > -	char shortOptions[optionsMap_.size() * 3 + 2];
> > -	struct option longOptions[optionsMap_.size() + 1];
> > +	char *shortOptions = (char*)alloca(optionsMap_.size() * 3 + 2);
> > +	struct option *longOptions = (struct option*)alloca(optionsMap_.size() + 1);
> 
> std::string usually has SSO, so that could work just fine here. But I suppose
> one really wants something like llvm::SmallVector.

That could be nice. Given that we're really not in a hot path, a
std::string (possibly with reserve()) is likely the best option here for
shortOptions, and std::vector should be fine for longOptions.

> >  	unsigned int ids = 0;
> >  	unsigned int idl = 0;
> > 
> > diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp
> > index 1980d374..3a7f8ee6 100644
> > --- a/src/libcamera/ipc_unixsocket.cpp
> > +++ b/src/libcamera/ipc_unixsocket.cpp
> > @@ -247,8 +247,8 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,
> >  	iov[0].iov_base = const_cast<void *>(buffer);
> >  	iov[0].iov_len = length;
> > 
> > -	char buf[CMSG_SPACE(num * sizeof(uint32_t))];
> > -	memset(buf, 0, sizeof(buf));
> > +	char *buf = (char*)malloc(CMSG_SPACE(num * sizeof(uint32_t)));
> > +	memset((void*)buf, 0, sizeof(buf));
> 
> The linux kernel has had a limit of 253 file descriptors for the last 13 years,

Does it ?

$ ulimit -Hn
4096
$ ulimit -Sn
1024

We should never have a large number of file descriptors here though, so
a fixed limit should be fine (I think 16 should be more than enough),
with proper error checking and handling, as well as a mention in the
documentation.

Paul, what do you think ?

> so I think a fixed size array is fine here.
> 
> >  	struct cmsghdr *cmsg = (struct cmsghdr *)buf;
> >  	cmsg->cmsg_len = CMSG_LEN(num * sizeof(uint32_t));
> > @@ -270,9 +270,11 @@ int IPCUnixSocket::sendData(const void *buffer, size_t length,
> >  		int ret = -errno;
> >  		LOG(IPCUnixSocket, Error)
> >  			<< "Failed to sendmsg: " << strerror(-ret);
> > +    free(buf);
> >  		return ret;
> >  	}
> > 
> > +  free(buf);
> >  	return 0;
> >  }
> > 
> > @@ -283,8 +285,8 @@ int IPCUnixSocket::recvData(void *buffer, size_t length,
> >  	iov[0].iov_base = buffer;
> >  	iov[0].iov_len = length;
> > 
> > -	char buf[CMSG_SPACE(num * sizeof(uint32_t))];
> > -	memset(buf, 0, sizeof(buf));
> > +	char *buf = (char*)malloc(CMSG_SPACE(num * sizeof(uint32_t)));
> > +	memset((void*)buf, 0, sizeof(buf));
> 
> Same here.
> 
> > 
> >  	struct cmsghdr *cmsg = (struct cmsghdr *)buf;
> >  	cmsg->cmsg_len = CMSG_LEN(num * sizeof(uint32_t));
> > @@ -305,12 +307,14 @@ int IPCUnixSocket::recvData(void *buffer, size_t length,
> >  		if (ret != -EAGAIN)
> >  			LOG(IPCUnixSocket, Error)
> >  				<< "Failed to recvmsg: " << strerror(-ret);
> > +    free(buf);
> >  		return ret;
> >  	}
> > 
> >  	if (fds)
> >  		memcpy(fds, CMSG_DATA(cmsg), num * sizeof(uint32_t));
> > 
> > +  free(buf);
> >  	return 0;
> >  }
> > 

-- 
Regards,

Laurent Pinchart