[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH v6 3/7] unshare: new applet
From:       Bartosz_Gołaszewski <bartekgola () gmail ! com>
Date:       2016-03-18 11:43:19
Message-ID: CAFdkumhGSH3GZVOTF6Gmvum7rD-zCfkk4UL_cF0dBaHePNDqTA () mail ! gmail ! com
[Download RAW message or body]

Hi Mike,

thanks for the reviews!

2016-03-17 18:23 GMT+01:00 Mike Frysinger <vapier@gentoo.org>:
> On 17 Mar 2016 15:52, Bartosz Golaszewski wrote:
>> +static void mount_procfs(const char *target)
>> +{
>> +     int status;
>> +
>> +     status = mount("none", target, NULL, MS_PRIVATE | MS_REC, NULL);
>> +     if (status < 0)
>> +             goto mount_err;
>> +
>> +     status = mount("proc", target, "proc",
>> +                    MS_NOSUID | MS_NOEXEC | MS_NODEV, NULL);
>
> each of these mount calls could do with a comment explaining what/why.
> you & i might understand how /proc needs to be made private & then
> freshly mounted in a new pid ns, but not everyone :).
>

Done in v7.

>> +     if (status < 0)
>> +             goto mount_err;
>
> general style note ... seems like this could be written:
>         status = mount(...)
>         if (status == 0)
>                 status = mount(...)
>         if (status < 0)
>                 bb_perror_msg_and_die(...)
>
> might be smaller code wise ?
>

Changed in v7.

>> +     run_shell(getenv("SHELL"), 0, NULL, NULL);
>
> if SHELL isn't set, then we just segfault ?

Nope, run_shell() checks if const char *shell is NULL or an empty
string and runs DEFAULT_SHELL in that case.

Tested on my regular Debian system:

bash$ ./busybox unshare
bash$
bash$ SHELL="" ./busybox unshare
sh-4.3$

-- 
Best regards,
Bartosz Golaszewski
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]