On 10/26/2018 03:00 PM, Ian Jackson wrote:
> Thanks, just tiny comments on this.
>
> George Dunlap writes ("[PATCH 3/5] tools/dm_restrict: Unshare mount and IPC
> namespaces on Linux"):
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 385643b52c..702ea75149 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -2393,6 +2393,12 @@ retry_transaction:
>> goto out_close;
>> if (!rc) { /* inner child */
>> setsid();
>> + if (libxl_defbool_val(b_info->dm_restrict))
>> + {
>
> Style: misplaced {.
>
>> + if (rc != 0)
>> + _exit(-1);
>
> I won't insist on a change here, since CODING_STYLE is not explicit,
> but you may want to take note of these statistics:
>
> $ git-grep 'if (rc)' tools/libxl | wc -l
> 520
> $ git-grep 'if (rc != 0)' tools/libxl | wc -l
> 23
>
>> +int libxl__local_dm_preexec_restrict(libxl__gc *gc)
>> +{
>> + int r;
>> +
>> + /* Unshare mount and IPC namespaces. These are unused by QEMU. */
>> + r = unshare(CLONE_NEWNS | CLONE_NEWIPC);
>> + if (r < 0) {
>> + LOGE(ERROR, "libxl: Mount and IPC namespace unfailed");
>
> I think I would slightly prefer
> + if (r) {
>
> unshare is supposed to return -1 or 0 so this should make no
> difference. If it does something else we should not carry on. It's
> unlikely enough that I don't mind it printing a garbage errno value in
> that case.
All three done.
> Acked-by: Ian Jackson <[email protected]>
Thanks,
-George
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
