| Message ID | 1366105701-7968-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/16/2013 03:48 AM, Kevin Wolf wrote: > $QEMU_PROG happens to be 'qemu' in my setup, so this sed command > replaces a bit too much. Restrict it to the start of the line and to > when it's followed by a colon, i.e. the form used by error messages. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > tests/qemu-iotests/common.filter | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter > index bc5f250..a7f889a 100644 > --- a/tests/qemu-iotests/common.filter > +++ b/tests/qemu-iotests/common.filter > @@ -155,7 +155,7 @@ _filter_qemu_io() > # replace occurrences of QEMU_PROG with "qemu" > _filter_qemu() > { > - sed -e "s#$(basename $QEMU_PROG)#QEMU_PROG#g" > + sed -e "s#^$(basename $QEMU_PROG):#QEMU_PROG:#g" Why spawn a basename process, when you can use shell to do the same? Also, the g modifier is worthless once you have a ^ anchor, since sed can only replace one ^ per line. And you don't need sed's -e option for a single script. The following is identical, with less typing: _filter_qemu() { sed "s#^${QEMU_PROG##*/}:#QEMU_PROG#" } That said, your more verbose version is still functionally correct, so you can add: Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue, Apr 16, 2013 at 06:26:38AM -0600, Eric Blake wrote: > On 04/16/2013 03:48 AM, Kevin Wolf wrote: > > $QEMU_PROG happens to be 'qemu' in my setup, so this sed command > > replaces a bit too much. Restrict it to the start of the line and to > > when it's followed by a colon, i.e. the form used by error messages. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > tests/qemu-iotests/common.filter | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter > > index bc5f250..a7f889a 100644 > > --- a/tests/qemu-iotests/common.filter > > +++ b/tests/qemu-iotests/common.filter > > @@ -155,7 +155,7 @@ _filter_qemu_io() > > # replace occurrences of QEMU_PROG with "qemu" > > _filter_qemu() > > { > > - sed -e "s#$(basename $QEMU_PROG)#QEMU_PROG#g" > > + sed -e "s#^$(basename $QEMU_PROG):#QEMU_PROG:#g" > > Why spawn a basename process, when you can use shell to do the same? > Also, the g modifier is worthless once you have a ^ anchor, since sed > can only replace one ^ per line. And you don't need sed's -e option for > a single script. sed -e is the convention in qemu-iotests. Dropping the 'g' would be nice. The problem with the POSIX shell string replacement is that the syntax is horrible. I can never remember what ${%}, ${%%}, ${#} and %{##} do. $(basename $QEMU_PROG) is clear (although it doesn't handle spaces in the filename!). Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Stefan Hajnoczi <stefanha@redhat.com> writes: [...] > The problem with the POSIX shell string replacement is that the syntax > is horrible. I can never remember what ${%}, ${%%}, ${#} and > %{##} do. $(basename $QEMU_PROG) is clear (although it doesn't handle > spaces in the filename!). Here's how I cope. # is left of % my keyboard. # matches "on the left", % "on the right". #/% are "short" and pick the shortest matching pattern. ##/%% are "long" and pick the longest matching pattern.
Am 16.04.2013 um 15:16 hat Stefan Hajnoczi geschrieben: > On Tue, Apr 16, 2013 at 06:26:38AM -0600, Eric Blake wrote: > > On 04/16/2013 03:48 AM, Kevin Wolf wrote: > > > $QEMU_PROG happens to be 'qemu' in my setup, so this sed command > > > replaces a bit too much. Restrict it to the start of the line and to > > > when it's followed by a colon, i.e. the form used by error messages. > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > --- > > > tests/qemu-iotests/common.filter | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter > > > index bc5f250..a7f889a 100644 > > > --- a/tests/qemu-iotests/common.filter > > > +++ b/tests/qemu-iotests/common.filter > > > @@ -155,7 +155,7 @@ _filter_qemu_io() > > > # replace occurrences of QEMU_PROG with "qemu" > > > _filter_qemu() > > > { > > > - sed -e "s#$(basename $QEMU_PROG)#QEMU_PROG#g" > > > + sed -e "s#^$(basename $QEMU_PROG):#QEMU_PROG:#g" > > > > Why spawn a basename process, when you can use shell to do the same? > > Also, the g modifier is worthless once you have a ^ anchor, since sed > > can only replace one ^ per line. And you don't need sed's -e option for > > a single script. > > sed -e is the convention in qemu-iotests. > > Dropping the 'g' would be nice. Okay, I just updated the commit to drop the 'g'. > The problem with the POSIX shell string replacement is that the syntax > is horrible. I can never remember what ${%}, ${%%}, ${#} and > %{##} do. $(basename $QEMU_PROG) is clear (although it doesn't handle > spaces in the filename!). That's exactly my problem as well. Kevin
On 04/16/2013 07:16 AM, Stefan Hajnoczi wrote: >>> + sed -e "s#^$(basename $QEMU_PROG):#QEMU_PROG:#g" >> >> Why spawn a basename process, when you can use shell to do the same? > The problem with the POSIX shell string replacement is that the syntax > is horrible. I can never remember what ${%}, ${%%}, ${#} and > %{##} do. $(basename $QEMU_PROG) is clear (although it doesn't handle > spaces in the filename!). As written, it also doesn't handle a leading '-' in the name. To be robust, it should be $(basename -- "$QEMU_PROG") - and even then, it still fails if $QEMU_PROG has a trailing newline (but no one is that perverse in how they name their program, right?). The shell variant never goofs on any of those corner cases. But I can totally understand your aversion to line noise, and as I'm doubtful that anyone will run the testsuite while trying to stress extreme corner-case $QEMU_PROG naming conventions, I can certainly live with keeping $(basename $QEMU_PROG) form for legibility.
On Tue, Apr 16, 2013 at 03:52:14PM +0200, Markus Armbruster wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > > [...] > > The problem with the POSIX shell string replacement is that the syntax > > is horrible. I can never remember what ${%}, ${%%}, ${#} and > > %{##} do. $(basename $QEMU_PROG) is clear (although it doesn't handle > > spaces in the filename!). > > Here's how I cope. # is left of % my keyboard. # matches "on the > left", % "on the right". #/% are "short" and pick the shortest matching > pattern. ##/%% are "long" and pick the longest matching pattern. Nice :). Stefan
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter index bc5f250..a7f889a 100644 --- a/tests/qemu-iotests/common.filter +++ b/tests/qemu-iotests/common.filter @@ -155,7 +155,7 @@ _filter_qemu_io() # replace occurrences of QEMU_PROG with "qemu" _filter_qemu() { - sed -e "s#$(basename $QEMU_PROG)#QEMU_PROG#g" + sed -e "s#^$(basename $QEMU_PROG):#QEMU_PROG:#g" } # make sure this script returns success
$QEMU_PROG happens to be 'qemu' in my setup, so this sed command replaces a bit too much. Restrict it to the start of the line and to when it's followed by a colon, i.e. the form used by error messages. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/common.filter | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)