Re: [Xen-devel] [PATCH 3/5] tools/dm_restrict: Unshare mount and IPC namespaces on Linux

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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.

Anyway,

Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

assuming you change at least the misplaced { which contradicts
CODING_STYLE.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel