Christian Brauner <christian.brau...@canonical.com> writes:
> On Wed, Oct 18, 2017 at 07:48:14PM -0500, Eric W. Biederman wrote:
>> Christian Brauner <christian.brau...@canonical.com> writes:
>>
>> > I'm not sure why the build is complaining about how the union is
>> > initialized
>> > here. This looks legitimate to me and I can't reproduce this locally with
>> > or
>> > without the appended config. The struct introduced here is:
>> >
>> > #define UID_GID_MAP_MAX_EXTENTS 5
>> >
>> > struct uid_gid_extent {
>> > u32 first;
>> > u32 lower_first;
>> > u32 count;
>> > };
>> >
>> > struct uid_gid_map { /* 64 bytes -- 1 cache line */
>> > u32 nr_extents;
>> > union {
>> > struct uid_gid_extent extent[UID_GID_MAP_MAX_EXTENTS];
>> > struct {
>> > struct uid_gid_extent *forward;
>> > struct uid_gid_extent *reverse;
>> > };
>> > };
>> > };
>> >
>> > And the initialization in kernel/user.c which I didn't change looks
>> > correct.
>> > But maybe I'm missing the point.
>>
>> You may want to check your compiler version this feels like a compiler
>> dependent error.
>>
>> It looks like gcc isn't happy about not having braces for the anonymous
>> union of extent and the anonymouns structure that holds forward and
>> reverse.
>>
>> FYI since I am commenting. I took a quick skim through your code today
>> and at first glance everything looks good. The performance is nice and
>> fast, and the changes look reasonable at first glance.
>
> Thanks. Glad to hear.
>
>>
>> I think there are some nits that can be picked but nothing yet that
>> indicates the code is working incorrectly.
>
> Do you want me to wait for your feedback? If not I'd send a new version of the
> patch with an additonal patch for kernel/user.c to use enclosing brackets when
> initializing the union in the struct.
Please do.
The only solid feedback I have at this point is that you don't need
to take userns_state_mutex on free. As there are no references at that
point a lock isn't going to make a difference.
I think I may have seen a few extra smp_rmb() in there. But I have not
looked closely enough to confirm that.
But all of those are the code works, there is just a little room for
improvement kind of things. There is nothing in there (except the
kernel/user.c initialization) that I have seen so far that says it could
not be merged now.
Eric
