[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on erro

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH] monitor: report entirety of hmp command on error
Date: Fri, 4 May 2018 13:19:37 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0

On 05/04/2018 01:02 PM, Collin Walling wrote:

So rather than trying to reconstruct a string, you could reuse what you already 
have.  This is a shorter patch that I think accomplishes the same goal:

diff --git i/monitor.c w/monitor.c
index 39f8ee17ba7..38736b3a20d 100644
--- i/monitor.c
+++ w/monitor.c
@@ -3371,6 +3371,7 @@ static void handle_hmp_command(Monitor *mon, const char 
      QDict *qdict;
      const mon_cmd_t *cmd;
+    const char *cmd_start = cmdline;

      trace_handle_hmp_command(mon, cmdline);

@@ -3381,8 +3382,11 @@ static void handle_hmp_command(Monitor *mon, const char 

      qdict = monitor_parse_arguments(mon, &cmdline, cmd);
      if (!qdict) {
-        monitor_printf(mon, "Try \"help %s\" for more information\n",
-                       cmd->name);
+        while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
+            cmdline--;
+        }
+        monitor_printf(mon, "Try \"help %.*s\" for more information\n",
+                       (int)(cmdline - cmd_start), cmd_start);

Very interesting... you managed to reuse what was in cmdline without printing 
anything extraneous that
the user might have provided... nicely done!

Your print statement is intriguing to me... I'm not entirely sure how it works.

The format specifiers in printf are %[flags][width][.precision]format. So I'm requesting a precision-limited string print (which says the maximum number of characters to print, rather than the usual semantics of printing until the trailing NUL is found), and the precision of .* (instead of a more typical .1 or similar) says that the precision will be an int argument to printf rather than inline (the width argument can also be passed via *). The cast to int is annoyingly part of the specification (subtracting two pointers within a string results in a ptrdiff_t, but on 64-bit platforms, ptrdiff_t and int are not equally handled through vararg functions). And that exact style of printf() magic was already in use in monitor_parse_command() that your patch attempt was touching (see where cmdp_start is used). And if you really want weird, 'man 3 printf' states that "%.*s" is equivalent to "%2$.*1$s" (the $ syntax is for cases cases where you need to consume printf arguments out-of-order, often when dealing with translated strings).

How would you like to move forward with this patch?

I'm more than happy to let you post a v2 of the patch, incorporating the ideas you just learned from me. (Or, if you do nothing, then in a week or so, I'll probably notice the patch is still sitting unapplied in my local repository and submit it myself - but then I wouldn't be helping the qemu community grow...)

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

reply via email to

[Prev in Thread] Current Thread [Next in Thread]