[PATCH v1] configure: judge build dir permission

[PATCH v1] configure: judge build dir permission

[Guo Zhi]

If this patch is applied, issue:

https://gitlab.com/qemu-project/qemu/-/issues/321

can be closed.

Signed-off-by: Guo Zhi <qtxuning1999@sjtu.edu.cn>
---
 configure | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 7c08c18358..9cfa78efd2 100755
--- a/configure
+++ b/configure
@@ -24,7 +24,13 @@ then
     then
         if test -f $MARKER
         then
-           rm -rf build
+            if test -w $MARKER
+            then
+                rm -rf build
+            else
+                echo "ERROR: ./build dir already exists and can not be removed due to permission"
+                exit 1
+            fi
         else
             echo "ERROR: ./build dir already exists and was not previously created by configure"
             exit 1
-- 
2.35.1

Re: [PATCH v1] configure: judge build dir permission

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]

On Tue, Apr 05, 2022 at 09:48:20PM +0800, Guo Zhi wrote:
> If this patch is applied, issue:
> 
> https://gitlab.com/qemu-project/qemu/-/issues/321
> 
> can be closed.
> 
> Signed-off-by: Guo Zhi <qtxuning1999@sjtu.edu.cn>
> ---
>  configure | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 7c08c18358..9cfa78efd2 100755
> --- a/configure
> +++ b/configure
> @@ -24,7 +24,13 @@ then
>      then
>          if test -f $MARKER
>          then
> -           rm -rf build
> +            if test -w $MARKER
> +            then
> +                rm -rf build
> +            else
> +                echo "ERROR: ./build dir already exists and can not be removed due to permission"
> +                exit 1
> +            fi

Other cases where "rm -rf build" fails are ignored. The script will keep
running and produce confusing output.

Maybe the script should check if rm(1) failed so that all possible cases
where the build directory is broken produce reasonable error messages?

Then there is also no need to check $MARKER explicitly.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] configure: judge build dir permission

[Peter Maydell]

On Wed, 6 Apr 2022 at 16:37, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Tue, Apr 05, 2022 at 09:48:20PM +0800, Guo Zhi wrote:
> > If this patch is applied, issue:
> >
> > https://gitlab.com/qemu-project/qemu/-/issues/321
> >
> > can be closed.
> >
> > Signed-off-by: Guo Zhi <qtxuning1999@sjtu.edu.cn>
> > ---
> >  configure | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/configure b/configure
> > index 7c08c18358..9cfa78efd2 100755
> > --- a/configure
> > +++ b/configure
> > @@ -24,7 +24,13 @@ then
> >      then
> >          if test -f $MARKER
> >          then
> > -           rm -rf build
> > +            if test -w $MARKER
> > +            then
> > +                rm -rf build
> > +            else
> > +                echo "ERROR: ./build dir already exists and can not be removed due to permission"
> > +                exit 1
> > +            fi
>
> Other cases where "rm -rf build" fails are ignored. The script will keep
> running and produce confusing output.
>
> Maybe the script should check if rm(1) failed so that all possible cases
> where the build directory is broken produce reasonable error messages?
>
> Then there is also no need to check $MARKER explicitly.

That isn't sufficient for the situation described in the
issue (and nor is this patch, I think) -- there what
happens is that because the source directory tree is not
owned by the user running configure, the "mkdir build"
command fails. Execution never goes into the "then"
that this patch is changing.

If we check that the mkdir succeeds then we don't really
need to check whether the rm -rf succeeded (though it
might be nice to do so anyway), because if the rm fails
then so will the mkdir (because the directory already
exists).

We should also test whether we handle the "build directory
not writeable" case for when we're not doing this
"create ./build because the user ran configure in the
source directory" thing (ie when configure is run
from a separate builddir).

-- PMM