Re: [PATCH kvmtool v3 3/9] update_headers.sh: arm64: Copy sve_context.h if available

[Dave Martin <Dave.Martin@arm.com>]

On Fri, May 31, 2019 at 06:03:40PM +0100, Andre Przywara wrote:
> On Thu, 30 May 2019 16:13:08 +0100
> Dave Martin <Dave.Martin@arm.com> wrote:
> 
> > The SVE KVM support for arm64 includes the additional backend
> > header <asm/sve_context.h> from <asm/kvm.h>.
> > 
> > So update this header if it is available.
> > 
> > To avoid creating a sudden dependency on a specific minimum kernel
> > version, ignore the header if the source kernel tree doesn't have
> > it.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  util/update_headers.sh | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/util/update_headers.sh b/util/update_headers.sh
> > index a7e21b8..90d3ead 100755
> > --- a/util/update_headers.sh
> > +++ b/util/update_headers.sh
> > @@ -25,11 +25,22 @@ fi
> >  
> >  cp -- "$LINUX_ROOT/include/uapi/linux/kvm.h" include/linux
> >  
> > +unset KVMTOOL_PATH
> > +
> > +copy_arm64 () {
> > +	local src=$LINUX_ROOT/arch/$arch/include/uapi/asm/sve_context.h
> 
> To go with your previous patches, aren't you missing the quotes here?

Hmmm, good question.  This is "obviously" a fancy variable assignment,
and so there would be no word splitting after expansion.  So quotes
wouldn't be needed here, just as with a simple assignment.  bash and
ash seem to work this way.

dash doesn't though, and a padantic reading of the bash man page
suggests that the dash behaviour may be more correct: i.e., local
is just a command, whose arguments are expanded in the usual way,
even if it happens to assign variables as part of its behaviour.

So, while I'm not sure whether or not quotes are officially needed here,
I guess we should have them to be on the safe side.

> > +
> > +	if [ -e "$src" ]
> > +	then
> > +		cp -- "$src" "$KVMTOOL_PATH/include/asm"
> > +	fi
> > +}
> > +
> 
> Maybe we can make this slightly more generic?
> copy_optional_arch() {
> 	local src="$LINUX_ROOT/arch/$arch/include/uapi/$1"
> 	[ -r "$src" ] && cp -- "$src" "$KVMTOOL_PATH/include/asm"
> }
> ...
> 	arm64) KVMTOOL_PATH=arm/aarch64
> 	       copy_optional_arch asm/sve_context.h
> 	       ;;

Happy to change it along those lines.  It's certainly possible this will
be needed again later for some future arch header.

Also, foo && bar exits the shell if foo yields false and set -e is in
effect, so I've reverted back to using an if.

(I'm still a little confused though, since I struggled to reproduce this
behaviour outside the script.)

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm