[PATCH v2 0/6] linux-user mmap debug cleanup

[PATCH v2 0/6] linux-user mmap debug cleanup

[Alex Bennée]

Hi,

This is a quick re-spin of the mmap tracing. I don't bother with
pretty formatting the protection/type flags as they were incomplete
and not hard to extract from just dumping the hex values. This makes
most of the patches much simpler. I've also dropped the home-made
pattern_glob routine.

The following patches need review:
   01 - linux user convert target_mprotect debug to trace
   02 - linux user convert target_mmap debug to tracepoin
   04 - linux user log page table changes under d page
   06 - trace replace hand crafted pattern_glob with g_pa

Alex Bennée (6):
  linux-user: convert target_mprotect debug to tracepoint
  linux-user: convert target_mmap debug to tracepoint
  linux-user: add target_mmap_complete tracepoint
  linux-user: log page table changes under -d page
  linux-user: convert target_munmap debug to a tracepoint
  trace: replace hand-crafted pattern_glob with g_pattern_match_simple

 include/exec/log.h      |  5 +++-
 bsd-user/main.c         |  2 +-
 linux-user/main.c       |  2 +-
 linux-user/mmap.c       | 56 ++++++++---------------------------------
 trace/control.c         | 35 +-------------------------
 linux-user/trace-events |  6 +++++
 6 files changed, 23 insertions(+), 83 deletions(-)

-- 
2.20.1

[PATCH v2 1/6] linux-user: convert target_mprotect debug to tracepoint

[Alex Bennée]

It is a pain to re-compile when you need to debug and tracepoints are
a fairly low impact way to instrument QEMU.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - don't both with ascii conversion
---
 linux-user/mmap.c       | 10 ++--------
 linux-user/trace-events |  3 +++
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 46a6e3a761a..26a83e74069 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -17,7 +17,7 @@
  *  along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
-
+#include "trace.h"
 #include "qemu.h"
 
 //#define DEBUG_MMAP
@@ -66,13 +66,7 @@ int target_mprotect(abi_ulong start, abi_ulong len, int prot)
     abi_ulong end, host_start, host_end, addr;
     int prot1, ret;
 
-#ifdef DEBUG_MMAP
-    printf("mprotect: start=0x" TARGET_ABI_FMT_lx
-           "len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c\n", start, len,
-           prot & PROT_READ ? 'r' : '-',
-           prot & PROT_WRITE ? 'w' : '-',
-           prot & PROT_EXEC ? 'x' : '-');
-#endif
+    trace_target_mprotect(start, len, prot);
 
     if ((start & ~TARGET_PAGE_MASK) != 0)
         return -TARGET_EINVAL;
diff --git a/linux-user/trace-events b/linux-user/trace-events
index 6df234bbb67..8419243de4e 100644
--- a/linux-user/trace-events
+++ b/linux-user/trace-events
@@ -11,3 +11,6 @@ user_handle_signal(void *env, int target_sig) "env=%p signal %d"
 user_host_signal(void *env, int host_sig, int target_sig) "env=%p signal %d (target %d("
 user_queue_signal(void *env, int target_sig) "env=%p signal %d"
 user_s390x_restore_sigregs(void *env, uint64_t sc_psw_addr, uint64_t env_psw_addr) "env=%p frame psw.addr 0x%"PRIx64 " current psw.addr 0x%"PRIx64
+
+# mmap.c
+target_mprotect(uint64_t start, uint64_t len, int flags) "start=0x%"PRIx64 " len=0x%"PRIx64 " prot=0x%x"
-- 
2.20.1

Re: [PATCH v2 1/6] linux-user: convert target_mprotect debug to tracepoint

[Richard Henderson]

On 12/5/19 4:25 AM, Alex Bennée wrote:
> It is a pain to re-compile when you need to debug and tracepoints are
> a fairly low impact way to instrument QEMU.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>   - don't both with ascii conversion
> ---
>  linux-user/mmap.c       | 10 ++--------
>  linux-user/trace-events |  3 +++
>  2 files changed, 5 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 1/6] linux-user: convert target_mprotect debug to tracepoint

[Laurent Vivier]

Le 05/12/2019 à 13:25, Alex Bennée a écrit :
> It is a pain to re-compile when you need to debug and tracepoints are
> a fairly low impact way to instrument QEMU.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>   - don't both with ascii conversion
> ---
>  linux-user/mmap.c       | 10 ++--------
>  linux-user/trace-events |  3 +++
>  2 files changed, 5 insertions(+), 8 deletions(-)

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH v2 2/6] linux-user: convert target_mmap debug to tracepoint

[Alex Bennée]

It is a pain to re-compile when you need to debug and tracepoints are
a fairly low impact way to instrument QEMU.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/mmap.c       | 27 +--------------------------
 linux-user/trace-events |  1 +
 2 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 26a83e74069..f4f10deaeac 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -363,32 +363,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
     abi_ulong ret, end, real_start, real_end, retaddr, host_offset, host_len;
 
     mmap_lock();
-#ifdef DEBUG_MMAP
-    {
-        printf("mmap: start=0x" TARGET_ABI_FMT_lx
-               " len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c flags=",
-               start, len,
-               prot & PROT_READ ? 'r' : '-',
-               prot & PROT_WRITE ? 'w' : '-',
-               prot & PROT_EXEC ? 'x' : '-');
-        if (flags & MAP_FIXED)
-            printf("MAP_FIXED ");
-        if (flags & MAP_ANONYMOUS)
-            printf("MAP_ANON ");
-        switch(flags & MAP_TYPE) {
-        case MAP_PRIVATE:
-            printf("MAP_PRIVATE ");
-            break;
-        case MAP_SHARED:
-            printf("MAP_SHARED ");
-            break;
-        default:
-            printf("[MAP_TYPE=0x%x] ", flags & MAP_TYPE);
-            break;
-        }
-        printf("fd=%d offset=" TARGET_ABI_FMT_lx "\n", fd, offset);
-    }
-#endif
+    trace_target_mmap(start, len, prot, flags, fd, offset);
 
     if (!len) {
         errno = EINVAL;
diff --git a/linux-user/trace-events b/linux-user/trace-events
index 8419243de4e..8d8d4c3c68c 100644
--- a/linux-user/trace-events
+++ b/linux-user/trace-events
@@ -14,3 +14,4 @@ user_s390x_restore_sigregs(void *env, uint64_t sc_psw_addr, uint64_t env_psw_add
 
 # mmap.c
 target_mprotect(uint64_t start, uint64_t len, int flags) "start=0x%"PRIx64 " len=0x%"PRIx64 " prot=0x%x"
+target_mmap(uint64_t start, uint64_t len, int pflags, int mflags, int fd, uint64_t offset) "start=0x%"PRIx64 " len=0x%"PRIx64 " prot=0x%x flags=0x%x fd=%d offset=0x%"PRIx64
-- 
2.20.1

Re: [PATCH v2 2/6] linux-user: convert target_mmap debug to tracepoint

[Richard Henderson]

On 12/5/19 4:25 AM, Alex Bennée wrote:
> It is a pain to re-compile when you need to debug and tracepoints are
> a fairly low impact way to instrument QEMU.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  linux-user/mmap.c       | 27 +--------------------------
>  linux-user/trace-events |  1 +
>  2 files changed, 2 insertions(+), 26 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 2/6] linux-user: convert target_mmap debug to tracepoint

[Laurent Vivier]

Le 05/12/2019 à 13:25, Alex Bennée a écrit :
> It is a pain to re-compile when you need to debug and tracepoints are
> a fairly low impact way to instrument QEMU.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  linux-user/mmap.c       | 27 +--------------------------
>  linux-user/trace-events |  1 +
>  2 files changed, 2 insertions(+), 26 deletions(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH v2 3/6] linux-user: add target_mmap_complete tracepoint

[Alex Bennée]

For full details we also want to see where the mmaps end up.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/mmap.c       | 2 +-
 linux-user/trace-events | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index f4f10deaeac..0b1b43ac3c0 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -538,8 +538,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
  the_end1:
     page_set_flags(start, start + len, prot | PAGE_VALID);
  the_end:
+    trace_target_mmap_complete(start);
 #ifdef DEBUG_MMAP
-    printf("ret=0x" TARGET_ABI_FMT_lx "\n", start);
     page_dump(stdout);
     printf("\n");
 #endif
diff --git a/linux-user/trace-events b/linux-user/trace-events
index 8d8d4c3c68c..6d6aeef7b52 100644
--- a/linux-user/trace-events
+++ b/linux-user/trace-events
@@ -15,3 +15,4 @@ user_s390x_restore_sigregs(void *env, uint64_t sc_psw_addr, uint64_t env_psw_add
 # mmap.c
 target_mprotect(uint64_t start, uint64_t len, int flags) "start=0x%"PRIx64 " len=0x%"PRIx64 " prot=0x%x"
 target_mmap(uint64_t start, uint64_t len, int pflags, int mflags, int fd, uint64_t offset) "start=0x%"PRIx64 " len=0x%"PRIx64 " prot=0x%x flags=0x%x fd=%d offset=0x%"PRIx64
+target_mmap_complete(uint64_t retaddr) "retaddr=0x%"PRIx64
-- 
2.20.1

Re: [PATCH v2 3/6] linux-user: add target_mmap_complete tracepoint

[Laurent Vivier]

Le 05/12/2019 à 13:25, Alex Bennée a écrit :
> For full details we also want to see where the mmaps end up.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/mmap.c       | 2 +-
>  linux-user/trace-events | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH v2 4/6] linux-user: log page table changes under -d page

[Alex Bennée]

The CPU_LOG_PAGE flag is woefully underused and could stand to do
extra duty tracking page changes. If the user doesn't want to see the
details as things change they still have the tracepoints available.

We push the locking into log_page_dump and pass a reason for the
banner text.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
  v2
    - reworded banner text
    - moved locking into helper
    - converted stray calls of page_dump
---
 include/exec/log.h | 5 ++++-
 bsd-user/main.c    | 2 +-
 linux-user/main.c  | 2 +-
 linux-user/mmap.c  | 8 ++++----
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/exec/log.h b/include/exec/log.h
index e2cfd436e61..012af09f9b3 100644
--- a/include/exec/log.h
+++ b/include/exec/log.h
@@ -50,9 +50,12 @@ static inline void log_disas(void *code, unsigned long size)
 
 #if defined(CONFIG_USER_ONLY)
 /* page_dump() output to the log file: */
-static inline void log_page_dump(void)
+static inline void log_page_dump(const char *operation)
 {
+    qemu_log_lock();
+    qemu_log("page layout changed following %s\n", operation);
     page_dump(qemu_logfile);
+    qemu_log_unlock();
 }
 #endif
 #endif
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 470a8bf79ed..7f4e3cd6271 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -963,7 +963,7 @@ int main(int argc, char **argv)
 
     if (qemu_loglevel_mask(CPU_LOG_PAGE)) {
         qemu_log("guest_base  0x%lx\n", guest_base);
-        log_page_dump();
+        log_page_dump("binary load");
 
         qemu_log("start_brk   0x" TARGET_ABI_FMT_lx "\n", info->start_brk);
         qemu_log("end_code    0x" TARGET_ABI_FMT_lx "\n", info->end_code);
diff --git a/linux-user/main.c b/linux-user/main.c
index 6ff7851e86f..8718d03ee21 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -826,7 +826,7 @@ int main(int argc, char **argv, char **envp)
 
     if (qemu_loglevel_mask(CPU_LOG_PAGE)) {
         qemu_log("guest_base  0x%lx\n", guest_base);
-        log_page_dump();
+        log_page_dump("binary load");
 
         qemu_log("start_brk   0x" TARGET_ABI_FMT_lx "\n", info->start_brk);
         qemu_log("end_code    0x" TARGET_ABI_FMT_lx "\n", info->end_code);
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 0b1b43ac3c0..3d90fa459ca 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -18,6 +18,7 @@
  */
 #include "qemu/osdep.h"
 #include "trace.h"
+#include "exec/log.h"
 #include "qemu.h"
 
 //#define DEBUG_MMAP
@@ -539,10 +540,9 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
     page_set_flags(start, start + len, prot | PAGE_VALID);
  the_end:
     trace_target_mmap_complete(start);
-#ifdef DEBUG_MMAP
-    page_dump(stdout);
-    printf("\n");
-#endif
+    if (qemu_loglevel_mask(CPU_LOG_PAGE)) {
+        log_page_dump(__func__);
+    }
     tb_invalidate_phys_range(start, start + len);
     mmap_unlock();
     return start;
-- 
2.20.1

Re: [PATCH v2 4/6] linux-user: log page table changes under -d page

[Richard Henderson]

On 12/5/19 4:25 AM, Alex Bennée wrote:
> The CPU_LOG_PAGE flag is woefully underused and could stand to do
> extra duty tracking page changes. If the user doesn't want to see the
> details as things change they still have the tracepoints available.
> 
> We push the locking into log_page_dump and pass a reason for the
> banner text.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
>   v2
>     - reworded banner text
>     - moved locking into helper
>     - converted stray calls of page_dump
> ---
>  include/exec/log.h | 5 ++++-
>  bsd-user/main.c    | 2 +-
>  linux-user/main.c  | 2 +-
>  linux-user/mmap.c  | 8 ++++----
>  4 files changed, 10 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 4/6] linux-user: log page table changes under -d page

[Laurent Vivier]

Le 05/12/2019 à 13:25, Alex Bennée a écrit :
> The CPU_LOG_PAGE flag is woefully underused and could stand to do
> extra duty tracking page changes. If the user doesn't want to see the
> details as things change they still have the tracepoints available.
> 
> We push the locking into log_page_dump and pass a reason for the
> banner text.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
>   v2
>     - reworded banner text
>     - moved locking into helper
>     - converted stray calls of page_dump
> ---
>  include/exec/log.h | 5 ++++-
>  bsd-user/main.c    | 2 +-
>  linux-user/main.c  | 2 +-
>  linux-user/mmap.c  | 8 ++++----
>  4 files changed, 10 insertions(+), 7 deletions(-)

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH v2 5/6] linux-user: convert target_munmap debug to a tracepoint

[Alex Bennée]

Convert the final bit of DEBUG_MMAP to a tracepoint and remove the
last remanents of the #ifdef hackery.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/mmap.c       | 9 ++-------
 linux-user/trace-events | 1 +
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 3d90fa459ca..8685f02e7e9 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -21,8 +21,6 @@
 #include "exec/log.h"
 #include "qemu.h"
 
-//#define DEBUG_MMAP
-
 static pthread_mutex_t mmap_mutex = PTHREAD_MUTEX_INITIALIZER;
 static __thread int mmap_lock_count;
 
@@ -597,11 +595,8 @@ int target_munmap(abi_ulong start, abi_ulong len)
     abi_ulong end, real_start, real_end, addr;
     int prot, ret;
 
-#ifdef DEBUG_MMAP
-    printf("munmap: start=0x" TARGET_ABI_FMT_lx " len=0x"
-           TARGET_ABI_FMT_lx "\n",
-           start, len);
-#endif
+    trace_target_munmap(start, len);
+
     if (start & ~TARGET_PAGE_MASK)
         return -TARGET_EINVAL;
     len = TARGET_PAGE_ALIGN(len);
diff --git a/linux-user/trace-events b/linux-user/trace-events
index 6d6aeef7b52..f6de1b8befc 100644
--- a/linux-user/trace-events
+++ b/linux-user/trace-events
@@ -16,3 +16,4 @@ user_s390x_restore_sigregs(void *env, uint64_t sc_psw_addr, uint64_t env_psw_add
 target_mprotect(uint64_t start, uint64_t len, int flags) "start=0x%"PRIx64 " len=0x%"PRIx64 " prot=0x%x"
 target_mmap(uint64_t start, uint64_t len, int pflags, int mflags, int fd, uint64_t offset) "start=0x%"PRIx64 " len=0x%"PRIx64 " prot=0x%x flags=0x%x fd=%d offset=0x%"PRIx64
 target_mmap_complete(uint64_t retaddr) "retaddr=0x%"PRIx64
+target_munmap(uint64_t start, uint64_t len) "start=0x%"PRIx64" len=0x%"PRIx64
-- 
2.20.1

Re: [PATCH v2 5/6] linux-user: convert target_munmap debug to a tracepoint

[Laurent Vivier]

Le 05/12/2019 à 13:25, Alex Bennée a écrit :
> Convert the final bit of DEBUG_MMAP to a tracepoint and remove the
> last remanents of the #ifdef hackery.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/mmap.c       | 9 ++-------
>  linux-user/trace-events | 1 +
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 5/6] linux-user: convert target_munmap debug to a tracepoint

[Alex Bennée]

Laurent Vivier <laurent@vivier.eu> writes:

> Le 05/12/2019 à 13:25, Alex Bennée a écrit :
>> Convert the final bit of DEBUG_MMAP to a tracepoint and remove the
>> last remanents of the #ifdef hackery.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  linux-user/mmap.c       | 9 ++-------
>>  linux-user/trace-events | 1 +
>>  2 files changed, 3 insertions(+), 7 deletions(-)
>> 
>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Are you going to take this series via your tree or would you like me to
put the PR together?

-- 
Alex Bennée

Re: [PATCH v2 5/6] linux-user: convert target_munmap debug to a tracepoint

[Laurent Vivier]

Le 16/12/2019 à 13:00, Alex Bennée a écrit :
> 
> Laurent Vivier <laurent@vivier.eu> writes:
> 
>> Le 05/12/2019 à 13:25, Alex Bennée a écrit :
>>> Convert the final bit of DEBUG_MMAP to a tracepoint and remove the
>>> last remanents of the #ifdef hackery.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>  linux-user/mmap.c       | 9 ++-------
>>>  linux-user/trace-events | 1 +
>>>  2 files changed, 3 insertions(+), 7 deletions(-)
>>>
>>
>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> 
> Are you going to take this series via your tree or would you like me to
> put the PR together?
> 

As you prefer.

Thanks,
Laurent

Re: [PATCH v2 5/6] linux-user: convert target_munmap debug to a tracepoint

[Alex Bennée]

Laurent Vivier <laurent@vivier.eu> writes:

> Le 16/12/2019 à 13:00, Alex Bennée a écrit :
>> 
>> Laurent Vivier <laurent@vivier.eu> writes:
>> 
>>> Le 05/12/2019 à 13:25, Alex Bennée a écrit :
>>>> Convert the final bit of DEBUG_MMAP to a tracepoint and remove the
>>>> last remanents of the #ifdef hackery.
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>> ---
>>>>  linux-user/mmap.c       | 9 ++-------
>>>>  linux-user/trace-events | 1 +
>>>>  2 files changed, 3 insertions(+), 7 deletions(-)
>>>>
>>>
>>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>> 
>> Are you going to take this series via your tree or would you like me to
>> put the PR together?
>> 
>
> As you prefer.

Very well queued to pullreq/testing-logging-misc, thanks.

>
> Thanks,
> Laurent


-- 
Alex Bennée

[PATCH v2 6/6] trace: replace hand-crafted pattern_glob with g_pattern_match_simple

[Alex Bennée]

We already use g_pattern_match elsewhere so remove the duplication.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 trace/control.c | 35 +----------------------------------
 1 file changed, 1 insertion(+), 34 deletions(-)

diff --git a/trace/control.c b/trace/control.c
index d9cafc161bb..0fb81241607 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -98,38 +98,6 @@ TraceEvent *trace_event_name(const char *name)
     return NULL;
 }
 
-static bool pattern_glob(const char *pat, const char *ev)
-{
-    while (*pat != '\0' && *ev != '\0') {
-        if (*pat == *ev) {
-            pat++;
-            ev++;
-        }
-        else if (*pat == '*') {
-            if (pattern_glob(pat, ev+1)) {
-                return true;
-            } else if (pattern_glob(pat+1, ev)) {
-                return true;
-            } else {
-                return false;
-            }
-        } else {
-            return false;
-        }
-    }
-
-    while (*pat == '*') {
-        pat++;
-    }
-
-    if (*pat == '\0' && *ev == '\0') {
-        return true;
-    } else {
-        return false;
-    }
-}
-
-
 void trace_event_iter_init(TraceEventIter *iter, const char *pattern)
 {
     iter->event = 0;
@@ -148,8 +116,7 @@ TraceEvent *trace_event_iter_next(TraceEventIter *iter)
             iter->group++;
         }
         if (!iter->pattern ||
-            pattern_glob(iter->pattern,
-                         trace_event_get_name(ev))) {
+            g_pattern_match_simple(iter->pattern, trace_event_get_name(ev))) {
             return ev;
         }
     }
-- 
2.20.1

Re: [PATCH v2 6/6] trace: replace hand-crafted pattern_glob with g_pattern_match_simple

[Richard Henderson]

On 12/5/19 4:25 AM, Alex Bennée wrote:
> We already use g_pattern_match elsewhere so remove the duplication.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  trace/control.c | 35 +----------------------------------
>  1 file changed, 1 insertion(+), 34 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 6/6] trace: replace hand-crafted pattern_glob with g_pattern_match_simple

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Thu, Dec 05, 2019 at 12:25:17PM +0000, Alex Bennée wrote:
> We already use g_pattern_match elsewhere so remove the duplication.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  trace/control.c | 35 +----------------------------------
>  1 file changed, 1 insertion(+), 34 deletions(-)

Is g_pattern_match() a superset of pattern_glob()?  Existing patterns
should continue to work.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 6/6] trace: replace hand-crafted pattern_glob with g_pattern_match_simple

[Alex Bennée]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Thu, Dec 05, 2019 at 12:25:17PM +0000, Alex Bennée wrote:
>> We already use g_pattern_match elsewhere so remove the duplication.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  trace/control.c | 35 +----------------------------------
>>  1 file changed, 1 insertion(+), 34 deletions(-)
>
> Is g_pattern_match() a superset of pattern_glob()?  Existing patterns
> should continue to work.

Yes - it supports more than pattern_glob and a bit less than the system
glob():

  The g_pattern_match* functions match a string against a pattern
  containing '*' and '?' wildcards with similar semantics as the standard
  glob() function: '*' matches an arbitrary, possibly empty, string, '?'
  matches an arbitrary character.

  Note that in contrast to glob(), the '/' character can be matched by the
  wildcards, there are no '[...]' character ranges and '*' and '?' can not
  be escaped to include them literally in a pattern.

If you give me some example existing pattern forms we can add them to
test-logging. I manually tested both single and double * patterns while
working on the rest of the series.

>
> Stefan


--
Alex Bennée

Re: [PATCH v2 6/6] trace: replace hand-crafted pattern_glob with g_pattern_match_simple

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

On Thu, Dec 05, 2019 at 12:25:17PM +0000, Alex Bennée wrote:
> We already use g_pattern_match elsewhere so remove the duplication.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  trace/control.c | 35 +----------------------------------
>  1 file changed, 1 insertion(+), 34 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]