[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <20220506190716.0d0483778c4bf15daddc22a3@zhasha.com>
Date: Fri, 6 May 2022 19:07:16 +0200
From: Joakim Sindholt <opensource@...sha.com>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] pack posix_spawn file actions into extraneous
padding
On Wed, 4 May 2022 16:10:10 -0400, Rich Felker <dalias@...c.org> wrote:
> On Fri, Feb 04, 2022 at 08:09:13PM +0100, Joakim Sindholt wrote:
> > On Sat, Jan 29, 2022 at 02:43:28PM +0100, Joakim Sindholt wrote:
> > > I've written this patch that packs posix_spawn file actions into the
> > > posix_spawn_file_actions struct padding. I'm not sure if the way I've
> > > split the code up is desirable. Maybe the faexpand() function needs a
> > > different name and to be in its own .c file; or maybe a different
> > > approach is preferable.
>
> (I noted on IRC that we could also solve the problem of the "density"
> of the coding space by using the 32 bits of __pad0[1] to store type
> information for the 16 slots, but this does not seem necessary.)
Since close and dup2 are as densely packed as possible without getting
into bit packing, and those two are by far the most common cases, and
since open, the only remaining action that you might reasonably want to
call many times, also requires malloc every time (for the path), I don't
see a need to add complexity to the reader.
We should also consider the complexity of adding new actions.
> One change I think I would like to make is not using the names __pad0
> and __pad directly in code. It's ugly. Instead, fdop.h could do:
>
> #define fa_cnt __pad0[0]
> #define fa_ops __pad
>
> It might also make sense to define a macro for the fa_ops array limit
>
> #define FA_OPS_MAX_CNT ...
>
> using sizeof on a compound literal as appropriate.
I agree and I have integrated all of these into this new patch. On
multiple occasions I had to hunt down bugs related to confusing __pad
and __pad0 but I wasn't sure if this solution was acceptable.
> We could also for consistency (not using __ junk names from the public
> header directly) do
>
> #define fa_chain __actions
>
> Not sure if that's the best name for it but it seems to make things
> make more sense than writing __actions in the code that's no longer
> using the pointed-to data as the actions list.
Visually it's nicer just to be consistent with the other 2 fields being
fa_*.
Furthermore I have split faexpand into __faexpand in its own .c file to
avoid increasing the .text size of all the add* functions.
> I think I'm happy at this point with just doing "strace tests", but
> sometime between now and next release I would like to get a better set
> of tests into libc-test that cover usage of sufficiently many ops to
> require allocations. Something like a test function that takes a list
> of operations and builds both the file actions object and a shell
> command to evaluate that the file actions were executed correctly,
> using:
>
> read x <&%d && test "$x" = %s
>
> and:
>
> ! 2>/dev/null <&%d
>
> to assert that each fd is either closed or reads the expected content
> (expected content could be temp files with different content in each).
Testing manually with strace is still a fair bit of work if you're
trying to hit the weird corner cases. I think it makes sense to just
make the tests now. It might take a little while before I get to it
though.
This patch still appears to work fine. I have also attached the skeleton
program I manually modify to test it.
Download attachment "0001-pack-posix_spawn-file-actions-into-extraneous-paddin.patch" of type "application/octet-stream" (12766 bytes)
View attachment "test.c" of type "text/plain" (1133 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
