diff mbox

[11/11] log: add "-d trace:PATTERN"

Message ID 1445850841-29510-2-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Oct. 26, 2015, 9:14 a.m. UTC 
From: Paolo Bonzini <pbonzini@redhat.com>

This is a bit easier to use than "-trace" if you are also enabling
other kinds of logging.  It is also more discoverable for experienced
QEMU users, and accessible from user-mode emulators.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 util/log.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Lluís Vilanova Oct. 26, 2015, 1:12 p.m. UTC | #1 
Denis V Lunev writes:

> From:  Paolo Bonzini <pbonzini@redhat.com>
> This is a bit easier to use than "-trace" if you are also enabling
> other kinds of logging.  It is also more discoverable for experienced
> QEMU users, and accessible from user-mode emulators.

I'm not sure this should be added, since the same functionality is also
available through "-trace enable=<pattern>" (and the shortcut "-trace
<pattern>").

Also, I'd rather fold event name discovery into "-trace enable=?" (and the
shortcut "-trace ?"), mimicking the format already available for CPUs ("-cpu
?").


Cheers,
  Lluis
Eric Blake Oct. 26, 2015, 5:07 p.m. UTC | #2 
On 10/26/2015 07:12 AM, Lluís Vilanova wrote:
> Denis V Lunev writes:
> 
>> From:  Paolo Bonzini <pbonzini@redhat.com>
>> This is a bit easier to use than "-trace" if you are also enabling
>> other kinds of logging.  It is also more discoverable for experienced
>> QEMU users, and accessible from user-mode emulators.
> 
> I'm not sure this should be added, since the same functionality is also
> available through "-trace enable=<pattern>" (and the shortcut "-trace
> <pattern>").

Having more than one way to do something is not necessarily bad; it does
imply more maintenance to keep both ways working, but if one way is more
discoverable than the other it may be worth it.

> 
> Also, I'd rather fold event name discovery into "-trace enable=?" (and the
> shortcut "-trace ?"), mimicking the format already available for CPUs ("-cpu
> ?").
> 

If we do that, we should also support '-trace enable=help', because ? is
a shell metacharacter, and we have been moving towards using 'help'
rather than '?' to minimize the need for shell quoting when asking for help.
Lluís Vilanova Oct. 26, 2015, 7:23 p.m. UTC | #3 
Eric Blake writes:

> On 10/26/2015 07:12 AM, Lluís Vilanova wrote:
>> Denis V Lunev writes:
>> 
>>> From:  Paolo Bonzini <pbonzini@redhat.com>
>>> This is a bit easier to use than "-trace" if you are also enabling
>>> other kinds of logging.  It is also more discoverable for experienced
>>> QEMU users, and accessible from user-mode emulators.
>> 
>> I'm not sure this should be added, since the same functionality is also
>> available through "-trace enable=<pattern>" (and the shortcut "-trace
>> <pattern>").

> Having more than one way to do something is not necessarily bad; it does
> imply more maintenance to keep both ways working, but if one way is more
> discoverable than the other it may be worth it.

Certainly true. I just find it confusing to have the same functionality
available through different forms.


>> 
>> Also, I'd rather fold event name discovery into "-trace enable=?" (and the
>> shortcut "-trace ?"), mimicking the format already available for CPUs ("-cpu
>> ?").
>> 

> If we do that, we should also support '-trace enable=help', because ? is
> a shell metacharacter, and we have been moving towards using 'help'
> rather than '?' to minimize the need for shell quoting when asking for help.

Oh, I wasn't aware of the "deprecation" of '?'. Then it certainly makes more
sense to use 'help'.


Thanks,
  Lluis
diff mbox

Patch

diff --git a/util/log.c b/util/log.c
index 5c641a0..2bcef95 100644
--- a/util/log.c
+++ b/util/log.c
@@ -19,6 +19,7 @@ 
 
 #include "qemu-common.h"
 #include "qemu/log.h"
+#include "trace/control.h"
 
 static char *logfilename;
 FILE *qemu_logfile;
@@ -154,6 +155,11 @@  int qemu_str_to_log_mask(const char *str)
             for (item = qemu_log_items; item->mask != 0; item++) {
                 mask |= item->mask;
             }
+#ifdef CONFIG_TRACE_LOG
+        } else if (strncmp(p, "trace:", 6) == 0 && p + 6 != p1) {
+            trace_enable_events(p + 6);
+            mask |= LOG_TRACE;
+#endif
         } else {
             for (item = qemu_log_items; item->mask != 0; item++) {
                 if (cmp1(p, p1 - p, item->name)) {
@@ -161,9 +167,9 @@  int qemu_str_to_log_mask(const char *str)
                 }
             }
             return 0;
+        found:
+            mask |= item->mask;
         }
-    found:
-        mask |= item->mask;
         if (*p1 != ',') {
             break;
         }
@@ -177,6 +183,10 @@  void qemu_print_log_usage(FILE *f)
     const QEMULogItem *item;
     fprintf(f, "Log items (comma separated):\n");
     for (item = qemu_log_items; item->mask != 0; item++) {
-        fprintf(f, "%-10s %s\n", item->name, item->help);
+        fprintf(f, "%-15s %s\n", item->name, item->help);
     }
+#ifdef CONFIG_TRACE_LOG
+    fprintf(f, "trace:PATTERN   enable trace events\n");
+    fprintf(f, "\nUse \"-d trace:help\" to get a list of trace events.\n\n");
+#endif
 }