Re: [PATCH v3 03/20] gdbstub: moving code so that it can be easier accessed from outside the gdbstub: fromhex and tohex functions moved to a cutils header. GDBRegisterState moved to gdbstub.h

[Alex Bennée]

Nicolas Eder <nicolas.eder@lauterbach.com> writes:

> ---
>  gdbstub/gdbstub.c      |  9 +--------
>  gdbstub/internals.h    | 26 --------------------------
>  include/cutils.h       | 30 ++++++++++++++++++++++++++++++
>  include/exec/gdbstub.h |  9 ++++++++-

Please split into the utils and the reg state patches as they are unrelated.

>  4 files changed, 39 insertions(+), 35 deletions(-)
>  create mode 100644 include/cutils.h
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 1e96a71c0c..c43ff89393 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -44,14 +44,7 @@
>  #include "exec/hwaddr.h"
>  
>  #include "internals.h"
> -
> -typedef struct GDBRegisterState {
> -    int base_reg;
> -    int num_regs;
> -    gdb_get_reg_cb get_reg;
> -    gdb_set_reg_cb set_reg;
> -    const char *xml;
> -} GDBRegisterState;
> +#include "cutils.h"
>  
>  GDBState gdbserver_state;
>  
> diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> index 465c24b36e..011e6f1858 100644
> --- a/gdbstub/internals.h
> +++ b/gdbstub/internals.h
> @@ -74,32 +74,6 @@ typedef struct GDBState {
>  /* lives in main gdbstub.c */
>  extern GDBState gdbserver_state;
>  
> -/*
> - * Inline utility function, convert from int to hex and back
> - */
> -
> -static inline int fromhex(int v)
> -{
> -    if (v >= '0' && v <= '9') {
> -        return v - '0';
> -    } else if (v >= 'A' && v <= 'F') {
> -        return v - 'A' + 10;
> -    } else if (v >= 'a' && v <= 'f') {
> -        return v - 'a' + 10;
> -    } else {
> -        return 0;
> -    }
> -}
> -
> -static inline int tohex(int v)
> -{
> -    if (v < 10) {
> -        return v + '0';
> -    } else {
> -        return v - 10 + 'a';
> -    }
> -}
> -
>  /*
>   * Connection helpers for both system and user backends
>   */
> diff --git a/include/cutils.h b/include/cutils.h
> new file mode 100644
> index 0000000000..a6b8dc3690
> --- /dev/null
> +++ b/include/cutils.h
> @@ -0,0 +1,30 @@
> +#ifndef CUTILS_H
> +#define CUTILS_H

We already have include/qemu/cutils.h where I think these can go.

> +
> +/*
> + * Inline utility function, convert from int to hex and back
> + */

Becoming common util functions they could do with a little cleaning up
before wider use.

  - kdoc comment string
  - rename to be clearer, maybe:
    - hexchar_to_nibble
    - nibble_to_hexchar
    

> +
> +static inline int fromhex(int v)


> +{
> +    if (v >= '0' && v <= '9') {
> +        return v - '0';
> +    } else if (v >= 'A' && v <= 'F') {
> +        return v - 'A' + 10;
> +    } else if (v >= 'a' && v <= 'f') {
> +        return v - 'a' + 10;
> +    } else {
> +        return 0;

g_assert_not_reached()? or document invalid chars are squashed to 0

> +    }
> +}
> +
> +static inline int tohex(int v)
> +{

g_assert(v =< 0xf)

> +    if (v < 10) {
> +        return v + '0';
> +    } else {
> +        return v - 10 + 'a';
> +    }
> +}
> +
> +#endif /* CUTILS_H */
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index f696e29477..cb70ebd7b4 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -15,11 +15,18 @@ typedef struct GDBFeature {
>      const char *xml;
>  } GDBFeature;
>  
> -
>  /* Get or set a register.  Returns the size of the register.  */
>  typedef int (*gdb_get_reg_cb)(CPUArchState *env, GByteArray *buf, int reg);
>  typedef int (*gdb_set_reg_cb)(CPUArchState *env, uint8_t *buf, int reg);
>  
> +typedef struct GDBRegisterState {
> +    int base_reg;
> +    int num_regs;
> +    gdb_get_reg_cb get_reg;
> +    gdb_set_reg_cb set_reg;
> +    const char *xml;
> +} GDBRegisterState;
> +
>  /**
>   * gdb_register_coprocessor() - register a supplemental set of registers
>   * @cpu - the CPU associated with registers

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro