On Wed, 6 Apr 2022 at 16:37, Stefan Hajnoczi <stefa...@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 <qtxuning1...@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
