| Message ID | 20190827131418.18713-1-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kernel-shark: Provide parsing for quotation marks in Record command line | expand |
Hi Yordan! Thanks for the quick work on this issue :) On Tue, Aug 27 16:14, Yordan Karadzhov (VMware) wrote: > Shell-like parsing of quotation marks in the content of the "Command" > field of the "Record" dialog will give more options to the users. > For example, now we can trace > > python -c 'print("hello world")' > > Suggested-by: Stephen Brennan <stephen@brennan.io> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204679 > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > kernel-shark/src/KsCaptureDialog.cpp | 38 ++++++++++++++++++++++++++-- > 1 file changed, 36 insertions(+), 2 deletions(-) > > diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp > index dc1e9b2..b3393b6 100644 > --- a/kernel-shark/src/KsCaptureDialog.cpp > +++ b/kernel-shark/src/KsCaptureDialog.cpp > @@ -157,7 +157,9 @@ KsCaptureControl::KsCaptureControl(QWidget *parent) > */ > QStringList KsCaptureControl::getArgs() > { > + QString::SplitBehavior opt = QString::SkipEmptyParts; > QStringList argv; > + QString cmdTmp; > > argv << "record"; > > @@ -170,7 +172,36 @@ QStringList KsCaptureControl::getArgs() > argv << _eventsWidget.getCheckedEvents(true); > > argv << "-o" << outputFileName(); > - argv << _commandLineEdit.text().split(" "); > + > + cmdTmp = _commandLineEdit.text(); > + if (!cmdTmp.contains('\'') && !cmdTmp.contains('\"')) { > + /* Split all command line arguments. */ > + argv << cmdTmp.split(" ", opt); > + } else { > + int iOpenQuots, iCloseQuots, size = cmdTmp.size(); > + int iSingleQuots = (cmdTmp.count('\'') == 2) ? cmdTmp.indexOf('\'') : size; > + int iDoubleQuots = (cmdTmp.count('\"') == 2) ? cmdTmp.indexOf('\"') : size; > + > + if (iSingleQuots < iDoubleQuots) { > + iOpenQuots = iSingleQuots; > + iCloseQuots = cmdTmp.lastIndexOf('\'') + 1; > + } else if (iDoubleQuots < iSingleQuots) { > + iOpenQuots = iDoubleQuots; > + iCloseQuots = cmdTmp.lastIndexOf('\"') + 1; > + } else { > + emit print("\nERROR: Unable to parse the command."); > + return {}; > + } > + > + /* Split the arguments. */ > + argv << cmdTmp.left(iOpenQuots).split(" ", opt); > + > + /* Everything in between the quotation marks goes in one piece. */ > + argv << cmdTmp.mid(iOpenQuots, iCloseQuots - iOpenQuots); > + > + /* Split the rest of the arguments. */ > + argv << cmdTmp.right(size - iCloseQuots).split(" ", opt); > + } It strikes me that this explicitly supports only a single set of quotes. This sort of behavior would be pretty surprising for people expecting shell quote support, and for people expecting just splitting on spaces. I looked and couldn't really find any Qt utility for properly parsing shell quoting (similar to python's shlex module). I totally get that it's a lot of work to implement a correct shell quoting parser. Maybe a compromise would be to add a checkbox to the capture dialog, which tells kernel-shark to pass the entire textbox contents, unmodified, to the shell implementation on the system. So, my example of: python -c 'print("hello world")' Would get put into the third argument of the command: /bin/sh -c INSERT_TEXTBOX_CONTENTS_HERE Then you could rely on /bin/sh doing the parsing for you. The downside is that it adds a whole new process. But you can't always get everything in life, right? Hope this was helpful! Stephen
On Tue, 27 Aug 2019 22:10:18 -0700 stephen@brennan.io wrote: > It strikes me that this explicitly supports only a single set of quotes. > This sort of behavior would be pretty surprising for people expecting shell > quote support, and for people expecting just splitting on spaces. > > I looked and couldn't really find any Qt utility for properly parsing shell > quoting (similar to python's shlex module). I totally get that it's a lot > of work to implement a correct shell quoting parser. > > Maybe a compromise would be to add a checkbox to the capture dialog, which > tells kernel-shark to pass the entire textbox contents, unmodified, to the > shell implementation on the system. So, my example of: > > python -c 'print("hello world")' > > Would get put into the third argument of the command: > > /bin/sh -c INSERT_TEXTBOX_CONTENTS_HERE > > Then you could rely on /bin/sh doing the parsing for you. The downside is > that it adds a whole new process. But you can't always get everything in > life, right? > I need to look at this a bit deeper. I've written lots of cases where I had to capture single and double quotes and turn them into a single command. This is definitely needed here. I'm very reluctant to just use a simple /bin/sh, as this is being run as root. Grant you, it is open to do anything, but I rather not just make it into a root shell. Although we still allow you to run any command. But once you add a full shell with the "sh -c" you now need to deal with environment variables and such, which can cause more unexpected side effects. Basically, we want to be able to add single and double quotes, as well as backslashes: my -c 'command\'s here' and '\\' this "too" Thanks for the feedback. -- Steve
On Tue, 27 Aug 2019 16:14:18 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > + cmdTmp = _commandLineEdit.text(); > + if (!cmdTmp.contains('\'') && !cmdTmp.contains('\"')) { > + /* Split all command line arguments. */ > + argv << cmdTmp.split(" ", opt); > + } else { > + int iOpenQuots, iCloseQuots, size = cmdTmp.size(); > + int iSingleQuots = (cmdTmp.count('\'') == 2) ? cmdTmp.indexOf('\'') : size; > + int iDoubleQuots = (cmdTmp.count('\"') == 2) ? cmdTmp.indexOf('\"') : size; > + > + if (iSingleQuots < iDoubleQuots) { > + iOpenQuots = iSingleQuots; > + iCloseQuots = cmdTmp.lastIndexOf('\'') + 1; > + } else if (iDoubleQuots < iSingleQuots) { > + iOpenQuots = iDoubleQuots; > + iCloseQuots = cmdTmp.lastIndexOf('\"') + 1; > + } else { > + emit print("\nERROR: Unable to parse the command."); > + return {}; > + } > + > + /* Split the arguments. */ > + argv << cmdTmp.left(iOpenQuots).split(" ", opt); > + > + /* Everything in between the quotation marks goes in one piece. */ > + argv << cmdTmp.mid(iOpenQuots, iCloseQuots - iOpenQuots); > + > + /* Split the rest of the arguments. */ > + argv << cmdTmp.right(size - iCloseQuots).split(" ", opt); > + } This is where I hate C++, because it makes simple things so complicated ;-) What we need to do is simply: char *str = _commandLineEdit.text(); char *last_word = str; char quote = 0; int i; // remove front and end spaces while (isspace(*str)) str++; i = strlen(str); while (i > 0 && isspace(str[i-1]) i--; str[i] = 0; for (i = 0; str[i]; i++) { if (isspace(str[i]) && !quote) { str[i++] = 0; argv << last_word; while (isspace(str[i])) i++; last_word = &str[i]; } switch(str[i]) { case '\\': i++; break; case '\'': case '"': if (!quote) quote = str[i]; else if (quote == str[i]) quote = 0; break; } } argv << last_word; Note, the above may be buggy, I didn't test it. -- Steve
On Wed, 28 Aug 2019 09:25:33 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Tue, 27 Aug 2019 16:14:18 +0300 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > > > + cmdTmp = _commandLineEdit.text(); > > + if (!cmdTmp.contains('\'') && !cmdTmp.contains('\"')) { > > + /* Split all command line arguments. */ > > + argv << cmdTmp.split(" ", opt); > > + } else { > > + int iOpenQuots, iCloseQuots, size = cmdTmp.size(); > > + int iSingleQuots = (cmdTmp.count('\'') == 2) ? cmdTmp.indexOf('\'') : size; > > + int iDoubleQuots = (cmdTmp.count('\"') == 2) ? cmdTmp.indexOf('\"') : size; > > + > > + if (iSingleQuots < iDoubleQuots) { > > + iOpenQuots = iSingleQuots; > > + iCloseQuots = cmdTmp.lastIndexOf('\'') + 1; > > + } else if (iDoubleQuots < iSingleQuots) { > > + iOpenQuots = iDoubleQuots; > > + iCloseQuots = cmdTmp.lastIndexOf('\"') + 1; > > + } else { > > + emit print("\nERROR: Unable to parse the command."); > > + return {}; > > + } > > + > > + /* Split the arguments. */ > > + argv << cmdTmp.left(iOpenQuots).split(" ", opt); > > + > > + /* Everything in between the quotation marks goes in one piece. */ > > + argv << cmdTmp.mid(iOpenQuots, iCloseQuots - iOpenQuots); > > + > > + /* Split the rest of the arguments. */ > > + argv << cmdTmp.right(size - iCloseQuots).split(" ", opt); > > + } > > This is where I hate C++, because it makes simple things so > complicated ;-) > > What we need to do is simply: > > char *str = _commandLineEdit.text(); > char *last_word = str; > char quote = 0; > int i; > > // remove front and end spaces > while (isspace(*str)) > str++; > i = strlen(str); > while (i > 0 && isspace(str[i-1]) > i--; > str[i] = 0; Oops, need to have: last_word = &str[i]; here. -- Steve > > for (i = 0; str[i]; i++) { > if (isspace(str[i]) && !quote) { > str[i++] = 0; > argv << last_word; > while (isspace(str[i])) > i++; > last_word = &str[i]; > } > switch(str[i]) { > case '\\': > i++; > break; > case '\'': > case '"': > if (!quote) > quote = str[i]; > else if (quote == str[i]) > quote = 0; > break; > } > } > argv << last_word; > > Note, the above may be buggy, I didn't test it. > > -- Steve
On 28.08.19 г. 16:06 ч., Steven Rostedt wrote: > On Tue, 27 Aug 2019 22:10:18 -0700 > stephen@brennan.io wrote: > >> It strikes me that this explicitly supports only a single set of quotes. >> This sort of behavior would be pretty surprising for people expecting shell >> quote support, and for people expecting just splitting on spaces. >> >> I looked and couldn't really find any Qt utility for properly parsing shell >> quoting (similar to python's shlex module). I totally get that it's a lot >> of work to implement a correct shell quoting parser. >> >> Maybe a compromise would be to add a checkbox to the capture dialog, which >> tells kernel-shark to pass the entire textbox contents, unmodified, to the >> shell implementation on the system. So, my example of: >> >> python -c 'print("hello world")' >> >> Would get put into the third argument of the command: >> >> /bin/sh -c INSERT_TEXTBOX_CONTENTS_HERE >> >> Then you could rely on /bin/sh doing the parsing for you. The downside is >> that it adds a whole new process. But you can't always get everything in >> life, right? >> > > I need to look at this a bit deeper. I've written lots of cases where I > had to capture single and double quotes and turn them into a single > command. This is definitely needed here. > Can you send me few of the of most trickier examples that comes to your mind, so that I can use them to test the parsing? Thanks! Yordan > I'm very reluctant to just use a simple /bin/sh, as this is being run > as root. Grant you, it is open to do anything, but I rather not just > make it into a root shell. Although we still allow you to run any > command. But once you add a full shell with the "sh -c" you now need to > deal with environment variables and such, which can cause more > unexpected side effects. > > Basically, we want to be able to add single and double quotes, as well > as backslashes: my -c 'command\'s here' and '\\' this "too" > > Thanks for the feedback. > > -- Steve >
On Wed, 28 Aug 2019 17:10:48 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > On 28.08.19 г. 16:06 ч., Steven Rostedt wrote: > > On Tue, 27 Aug 2019 22:10:18 -0700 > > stephen@brennan.io wrote: > > > >> It strikes me that this explicitly supports only a single set of quotes. > >> This sort of behavior would be pretty surprising for people expecting shell > >> quote support, and for people expecting just splitting on spaces. > >> > >> I looked and couldn't really find any Qt utility for properly parsing shell > >> quoting (similar to python's shlex module). I totally get that it's a lot > >> of work to implement a correct shell quoting parser. > >> > >> Maybe a compromise would be to add a checkbox to the capture dialog, which > >> tells kernel-shark to pass the entire textbox contents, unmodified, to the > >> shell implementation on the system. So, my example of: > >> > >> python -c 'print("hello world")' > >> > >> Would get put into the third argument of the command: > >> > >> /bin/sh -c INSERT_TEXTBOX_CONTENTS_HERE > >> > >> Then you could rely on /bin/sh doing the parsing for you. The downside is > >> that it adds a whole new process. But you can't always get everything in > >> life, right? > >> > > > > I need to look at this a bit deeper. I've written lots of cases where I > > had to capture single and double quotes and turn them into a single > > command. This is definitely needed here. > > > > Can you send me few of the of most trickier examples that comes to your > mind, so that I can use them to test the parsing? > I was thinking of when we start adding synthetic event code, we will definitely need to handle some of this. Like: --select 'irq_lat: lat=sched_waking.common_timestamp.usecs - hrtimer_start.common_timestamp.usecs, pid=sched_waking.pid from timer.hrtimer_start join sched.sched_waking on hrtimer_start.common_pid == sched_waking.common_pid where hrtimer_start.function == 0xffffffff81200580 and sched_waking.common_flags & 1' --select "wake_lat: lat=sched_switch.common_timestamp.usecs - irq_lat.common_timestamp.usecs, irqlat=irq_lat.lat, pid=sched_switch.next_pid from sched.sched_switch join irq_lat on sched_switch.next_pid == irq_lat.pip where next_comm == \"my-rt-code\"" Note, the above will be all on one single line! -- Steve
On 28.08.19 г. 14:30 ч., Steven Rostedt wrote: > On Wed, 28 Aug 2019 09:25:33 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > >> On Tue, 27 Aug 2019 16:14:18 +0300 >> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: >> >>> + cmdTmp = _commandLineEdit.text(); >>> + if (!cmdTmp.contains('\'') && !cmdTmp.contains('\"')) { >>> + /* Split all command line arguments. */ >>> + argv << cmdTmp.split(" ", opt); >>> + } else { >>> + int iOpenQuots, iCloseQuots, size = cmdTmp.size(); >>> + int iSingleQuots = (cmdTmp.count('\'') == 2) ? cmdTmp.indexOf('\'') : size; >>> + int iDoubleQuots = (cmdTmp.count('\"') == 2) ? cmdTmp.indexOf('\"') : size; >>> + >>> + if (iSingleQuots < iDoubleQuots) { >>> + iOpenQuots = iSingleQuots; >>> + iCloseQuots = cmdTmp.lastIndexOf('\'') + 1; >>> + } else if (iDoubleQuots < iSingleQuots) { >>> + iOpenQuots = iDoubleQuots; >>> + iCloseQuots = cmdTmp.lastIndexOf('\"') + 1; >>> + } else { >>> + emit print("\nERROR: Unable to parse the command."); >>> + return {}; >>> + } >>> + >>> + /* Split the arguments. */ >>> + argv << cmdTmp.left(iOpenQuots).split(" ", opt); >>> + >>> + /* Everything in between the quotation marks goes in one piece. */ >>> + argv << cmdTmp.mid(iOpenQuots, iCloseQuots - iOpenQuots); >>> + >>> + /* Split the rest of the arguments. */ >>> + argv << cmdTmp.right(size - iCloseQuots).split(" ", opt); >>> + } >> >> This is where I hate C++, because it makes simple things so >> complicated ;-) >> >> What we need to do is simply: >> >> char *str = _commandLineEdit.text(); >> char *last_word = str; >> char quote = 0; >> int i; >> >> // remove front and end spaces >> while (isspace(*str)) >> str++; >> i = strlen(str); >> while (i > 0 && isspace(str[i-1]) >> i--; >> str[i] = 0; > > Oops, need to have: > > last_word = &str[i]; > > here. > > -- Steve > >> >> for (i = 0; str[i]; i++) { >> if (isspace(str[i]) && !quote) { >> str[i++] = 0; >> argv << last_word; >> while (isspace(str[i])) >> i++; >> last_word = &str[i]; >> } >> switch(str[i]) { >> case '\\': >> i++; >> break; >> case '\'': >> case '"': >> if (!quote) >> quote = str[i]; >> else if (quote == str[i]) >> quote = 0; >> break; >> } >> } >> argv << last_word; >> >> Note, the above may be buggy, I didn't test it. >> Hi Steve, I understand very well your feelings ;) However, I also really dislike eclectic programming stiles. Since this is part of the C++/Qt code, I would prefer to stick to the C++/Qt way of doing things. Here is a version of your code that is more C++/Qt-ish: QString::SplitBehavior opt = QString::SkipEmptyParts; QChar quote = 0; int i, progress = 0, size; cmd.remove(QChar('\\')); size = cmd.count(); auto lamMid = [&] () {return cmd.mid(progress, i - progress);}; for (i = 0; i < size; ++i) { if (cmd[i] == '\'' || cmd[i] == '"') { if (quote.isNull()) { args << lamMid().split(" ", opt); quote = cmd[i++]; progress = i; } else if (quote == cmd[i]) { args << lamMid(); quote = 0; progress = ++i; } } } args << cmd.right(size - progress).split(" ", opt); If you are OK with this I will send a new version of the patch. Thanks! Yordan >> -- Steve >
On Tue, 17 Sep 2019 13:44:52 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >> This is where I hate C++, because it makes simple things so > >> complicated ;-) > >> > >> What we need to do is simply: > >> > >> char *str = _commandLineEdit.text(); > >> char *last_word = str; > >> char quote = 0; > >> int i; > >> > >> // remove front and end spaces > >> while (isspace(*str)) > >> str++; > >> i = strlen(str); > >> while (i > 0 && isspace(str[i-1]) > >> i--; > >> str[i] = 0; > > > > Oops, need to have: > > > > last_word = &str[i]; > > > > here. > > > > -- Steve > > > >> > >> for (i = 0; str[i]; i++) { > >> if (isspace(str[i]) && !quote) { > >> str[i++] = 0; > >> argv << last_word; > >> while (isspace(str[i])) > >> i++; > >> last_word = &str[i]; > >> } > >> switch(str[i]) { > >> case '\\': > >> i++; > >> break; > >> case '\'': > >> case '"': > >> if (!quote) > >> quote = str[i]; > >> else if (quote == str[i]) > >> quote = 0; > >> break; > >> } > >> } > >> argv << last_word; > >> > >> Note, the above may be buggy, I didn't test it. > >> > > Hi Steve, > > I understand very well your feelings ;) > However, I also really dislike eclectic programming stiles. Since this > is part of the C++/Qt code, I would prefer to stick to the C++/Qt way of > doing things. I'm fine with that as you will be maintaining it. But it also needs to be correct. > > Here is a version of your code that is more C++/Qt-ish: > > > QString::SplitBehavior opt = QString::SkipEmptyParts; > QChar quote = 0; > int i, progress = 0, size; > > cmd.remove(QChar('\\')); What does the above do? Removes all backslashes? Why? A backslashed quote must be ignored. '\'' is a single quote within quotes. Not to mention, a backslash keeps whatever it backslashed: \$ == $ \' == ' \" == " \1 == 1 \\ == \ I don't see how this code handles this, as my code does. -- Steve > size = cmd.count(); > auto lamMid = [&] () {return cmd.mid(progress, i - progress);}; > for (i = 0; i < size; ++i) { > if (cmd[i] == '\'' || cmd[i] == '"') { > if (quote.isNull()) { > args << lamMid().split(" ", opt); > quote = cmd[i++]; > progress = i; > } else if (quote == cmd[i]) { > args << lamMid(); > quote = 0; > progress = ++i; > } > } > } > > args << cmd.right(size - progress).split(" ", opt); > > If you are OK with this I will send a new version of the patch. > > Thanks! > Yordan > > > > >> -- Steve > >
diff --git a/kernel-shark/src/KsCaptureDialog.cpp b/kernel-shark/src/KsCaptureDialog.cpp index dc1e9b2..b3393b6 100644 --- a/kernel-shark/src/KsCaptureDialog.cpp +++ b/kernel-shark/src/KsCaptureDialog.cpp @@ -157,7 +157,9 @@ KsCaptureControl::KsCaptureControl(QWidget *parent) */ QStringList KsCaptureControl::getArgs() { + QString::SplitBehavior opt = QString::SkipEmptyParts; QStringList argv; + QString cmdTmp; argv << "record"; @@ -170,7 +172,36 @@ QStringList KsCaptureControl::getArgs() argv << _eventsWidget.getCheckedEvents(true); argv << "-o" << outputFileName(); - argv << _commandLineEdit.text().split(" "); + + cmdTmp = _commandLineEdit.text(); + if (!cmdTmp.contains('\'') && !cmdTmp.contains('\"')) { + /* Split all command line arguments. */ + argv << cmdTmp.split(" ", opt); + } else { + int iOpenQuots, iCloseQuots, size = cmdTmp.size(); + int iSingleQuots = (cmdTmp.count('\'') == 2) ? cmdTmp.indexOf('\'') : size; + int iDoubleQuots = (cmdTmp.count('\"') == 2) ? cmdTmp.indexOf('\"') : size; + + if (iSingleQuots < iDoubleQuots) { + iOpenQuots = iSingleQuots; + iCloseQuots = cmdTmp.lastIndexOf('\'') + 1; + } else if (iDoubleQuots < iSingleQuots) { + iOpenQuots = iDoubleQuots; + iCloseQuots = cmdTmp.lastIndexOf('\"') + 1; + } else { + emit print("\nERROR: Unable to parse the command."); + return {}; + } + + /* Split the arguments. */ + argv << cmdTmp.left(iOpenQuots).split(" ", opt); + + /* Everything in between the quotation marks goes in one piece. */ + argv << cmdTmp.mid(iOpenQuots, iCloseQuots - iOpenQuots); + + /* Split the rest of the arguments. */ + argv << cmdTmp.right(size - iCloseQuots).split(" ", opt); + } return argv; } @@ -350,7 +381,10 @@ void KsCaptureControl::_browse() void KsCaptureControl::_apply() { - emit argsReady(getArgs().join(" ")); + QStringList argv = getArgs(); + + if (argv.count()) + emit argsReady(argv.join(" ")); } /** @brief Create KsCaptureMonitor widget. */
Shell-like parsing of quotation marks in the content of the "Command" field of the "Record" dialog will give more options to the users. For example, now we can trace python -c 'print("hello world")' Suggested-by: Stephen Brennan <stephen@brennan.io> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204679 Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- kernel-shark/src/KsCaptureDialog.cpp | 38 ++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-)