Re: [Xen-devel] [PATCH v2 3/6] tools/dm_restrict: Ask QEMU to chroot

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

George Dunlap writes ("[PATCH v2 3/6] tools/dm_restrict: Ask QEMU to chroot"):
> When dm_restrict is enabled, ask QEMU to chroot into an empty directory.

This is great.  However,

1. Can you please arrange to install depriv-process-checker.sh ?
   That way osstest can run it.

2. depriv-process-checker.sh should be called depriv-process-checker
   so that we do not need to rename it if we change the implementation
   language.  (In general `foo.sh' `foo.pl' are a bad idea.)

3. If XEN_RUN_DIR is not set, that should be a test failure, not a
   silent "pass" although the test was not run, so I think it should
   exit nonzero.

4. Style:

> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index abd31ee6f2..df6fb64bee 100644
> -    if (libxl_defbool_val(b_info->dm_restrict))
> +    if (libxl_defbool_val(b_info->dm_restrict)) {
> +        char * chroot_dir = GCSPRINTF("%s/qemu-root-%d",
                 ^
Spurious space.

> +        if (rmdir(chroot_dir) != 0
> +            && errno != ENOENT) {

This error handling style is contrary to usual libxl practice.  More
idiomatic, and IMO more readable, would be something like this:

           r = rmdir(chroot_dir);
           if (r && errno != ENOENT) {

> +        for (;;) {
> +            if (!mkdir(chroot_dir, 0000))
> +                break;

Same here.

Thanks,
Ian.

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