[PATCH v5 1/5] PID namespace translation support

[Dmitry V. Levin]

On Wed, Aug 05, 2020 at 10:45:28AM +0200, Ákos Uzonyi wrote:
> On Wed, 5 Aug 2020 at 02:30, Dmitry V. Levin <ldv at altlinux.org> wrote:
> > On Wed, Aug 05, 2020 at 02:14:36AM +0200, Ákos Uzonyi wrote:
> > > On Wed, 5 Aug 2020 at 02:02, Dmitry V. Levin <ldv at altlinux.org> wrote:
> > > > On Wed, Aug 05, 2020 at 01:53:37AM +0200, Ákos Uzonyi wrote:
> > > > > On Wed, 5 Aug 2020 at 00:31, Dmitry V. Levin <ldv at altlinux.org> wrote:
> > > > > > On Tue, Aug 04, 2020 at 07:52:46PM +0200, Ákos Uzonyi wrote:
> > > > > > > On Mon, 3 Aug 2020 at 23:43, Dmitry V. Levin <ldv at altlinux.org> wrote:
> > > > > > > > On Mon, Aug 03, 2020 at 09:19:12PM +0200, Ákos Uzonyi wrote:
> > > > > > > > [...]
> > > > > > > > > +void
> > > > > > > > > +pidns_init(void)
> > > > > > > > > +{
> > > > > > > > > +     if (proc_data_cache)
> > > > > > > > > +             return;
> > > > > > > > > +
> > > > > > > > > +     pid_max = INT_MAX;
> > > > > > > > > +     if (read_int_from_file("/proc/sys/kernel/pid_max", &pid_max) < 0)
> > > > > > > > > +             debug_func_perror_msg("reading /proc/sys/kernel/pid_max");
> > > > > > > > > +     pid_max_size = ilog2_32(pid_max - 1) + 1;
> > > > > > > > > +     pid_max_size_lg = ilog2_32(pid_max_size - 1) + 1;
> > > > > > > > > +
> > > > > > > > > +     for (int i = 0; i < PT_COUNT; i++)
> > > > > > > > > +             ns_pid_to_proc_pid[i] = trie_create(64, ptr_sz_lg, 10, 10, 0);
> > > > > > > >
> > > > > > > > Has the numeric constant 64 been chosen because
> > > > > > > > sizeof(struct tcb.pid_ns) * 8 == 64?
> > > > > > >
> > > > > > > Yes.
> > > > > > > For making it clear in the code, which one would be preferred?
> > > > > > > - Using a named constant
> > > > > >
> > > > > > It's always nice to have a good name for a constant.
> > > > > >
> > > > > > > - Using the expression you wrote above
> > > > > >
> > > > > > Strictly speaking, that was not a valid C expression, but
> > > > > >         sizeof_field(struct tcb, pid_ns) * 8
> > > > > > is a valid one.
> > > > >
> > > > > As tcb.pid_ns is just one of the variables where ns ids are stored, I
> > > > > might prefer using sizeof(uint64_t):
> > > > > #define NS_ID_SIZE sizeof(uint64_t)
> > > > > or maybe just simply:
> > > > > #define NS_ID_SIZE 64
> > > >
> > > > By the way, is it really 64?  Yes, the type of struct tcb.pid_ns is 64-bit,
> > > > but any valid pid_ns is 32-bit.
> > >
> > > I think it was chosen to be 64 bit by Eugene, because it's not
> > > documented that it should fit into 32 bit. The documentation only says
> > > that it's derived from an inode number. Or is it guaranteed that inode
> > > numbers are maximum 32 bit?
> >
> > If I read the kernel correctly, pid_ns is assigned from struct ns_common.inum
> > of type unsigned int.
> 
> Yes, I understand. However since it's not documented anywhere (AFAIK),
> it might change later, isn't it? But if you say it's highly unlikely
> to change, we can use unsigned int for ns id type. I think we should
> not use -1U then for indicating error in tcb.pid_ns, as it is a likely
> valid ns id. It's not a big problem however, as errors are unlikely,
> so there is an option to just leave tcb.pid_ns 0 on error.

Yes, I agree, -1U shouldn't be used as an error indicator.


-- 
ldv