Re: [Qemu-devel] [PATCH v4 12/45] hw/dma: Replace fprintf(stderr, "*\n" with error_report()

[Philippe Mathieu-Daudé]

On 11/08/2017 07:57 PM, Alistair Francis wrote:
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
> 
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N; {s|fprintf(stderr, 
> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' 
> \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> 
> Some lines where then manually tweaked to pass checkpatch.
> 
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> V3:
>  - Remove ""s
> V2:
>  - Split hw patch into individual directories
> 
>  hw/dma/omap_dma.c | 26 ++++++++++++++------------
>  hw/dma/soc_dma.c  | 36 ++++++++++++++++++------------------
>  2 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/dma/omap_dma.c b/hw/dma/omap_dma.c
> index abd18c67ea..5d7fe06d98 100644
> --- a/hw/dma/omap_dma.c
> +++ b/hw/dma/omap_dma.c
> @@ -18,6 +18,7 @@
>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
>  #include "hw/arm/omap.h"
> @@ -1898,13 +1899,13 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
>              omap_dma_reset(s->dma);
>          s->ocp = value & 0x3321;
>          if (((s->ocp >> 12) & 3) == 3)                               /* 
> MIDLEMODE */
> -            fprintf(stderr, "%s: invalid DMA power mode\n", __func__);
> +            error_report("%s: invalid DMA power mode", __func__);
>          return;
>  
>      case 0x78:       /* DMA4_GCR */
>          s->gcr = value & 0x00ff00ff;
>       if ((value & 0xff) == 0x00)             /* MAX_CHANNEL_FIFO_DEPTH */
> -            fprintf(stderr, "%s: wrong FIFO depth in GCR\n", __func__);
> +            error_report("%s: wrong FIFO depth in GCR", __func__);
>          return;
>  
>      case 0x80 ... 0xfff:
> @@ -1934,8 +1935,8 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
>          ch->buf_disable = (value >> 25) & 1;
>          ch->src_sync = (value >> 24) & 1;    /* XXX For CamDMA must be 1 */
>          if (ch->buf_disable && !ch->src_sync)
> -            fprintf(stderr, "%s: Buffering disable is not allowed in "
> -                            "destination synchronised mode\n", __func__);
> +            error_report("%s: Buffering disable is not allowed in "
> +                         "destination synchronised mode", __func__);
>          ch->prefetch = (value >> 23) & 1;
>          ch->bs = (value >> 18) & 1;
>          ch->transparent_copy = (value >> 17) & 1;
> @@ -1946,8 +1947,8 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
>          ch->priority = (value & 0x0040) >> 6;
>          ch->fs = (value & 0x0020) >> 5;
>          if (ch->fs && ch->bs && ch->mode[0] && ch->mode[1])
> -            fprintf(stderr, "%s: For a packet transfer at least one port "
> -                            "must be constant-addressed\n", __func__);
> +            error_report("%s: For a packet transfer at least one port "
> +                         "must be constant-addressed", __func__);
>          ch->sync = (value & 0x001f) | ((value >> 14) & 0x0060);
>          /* XXX must be 0x01 for CamDMA */
>  
> @@ -1977,8 +1978,8 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
>          ch->endian[1] =(value >> 19) & 1;
>          ch->endian_lock[1] =(value >> 18) & 1;
>          if (ch->endian[0] != ch->endian[1])
> -            fprintf(stderr, "%s: DMA endianness conversion enable attempt\n",
> -                            __func__);
> +            error_report("%s: DMA endianness conversion enable attempt",
> +                          __func__);
>          ch->write_mode = (value >> 16) & 3;
>          ch->burst[1] = (value & 0xc000) >> 14;
>          ch->pack[1] = (value & 0x2000) >> 13;
> @@ -1986,12 +1987,13 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
>          ch->burst[0] = (value & 0x0180) >> 7;
>          ch->pack[0] = (value & 0x0040) >> 6;
>          ch->translate[0] = (value & 0x003c) >> 2;
> -        if (ch->translate[0] | ch->translate[1])
> -            fprintf(stderr, "%s: bad MReqAddressTranslate sideband signal\n",
> -                            __func__);
> +        if (ch->translate[0] | ch->translate[1]) {

This looks like bad coding of if (a || b) ...

> +            error_report("%s: bad MReqAddressTranslate sideband signal",
> +                         __func__);
> +        }
>          ch->data_type = 1 << (value & 3);
>          if ((value & 3) == 3) {
> -            printf("%s: bad data_type for DMA channel\n", __func__);
> +            error_report("%s: bad data_type for DMA channel", __func__);
>              ch->data_type >>= 1;
>          }
>          break;
> diff --git a/hw/dma/soc_dma.c b/hw/dma/soc_dma.c
> index 9bb499bf9c..45516241c6 100644
> --- a/hw/dma/soc_dma.c
> +++ b/hw/dma/soc_dma.c
> @@ -18,6 +18,7 @@
>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
>  #include "hw/arm/soc_dma.h"
> @@ -270,11 +271,11 @@ void soc_dma_port_add_fifo(struct soc_dma_s *soc, 
> hwaddr virt_base,
>          if (entry->type == soc_dma_port_mem) {
>              if (entry->addr <= virt_base &&
>                              entry->addr + entry->u.mem.size > virt_base) {
> -                fprintf(stderr, "%s: FIFO at %"PRIx64
> -                                " collides with RAM region at %"PRIx64
> -                                "-%"PRIx64 "\n", __func__,
> -                                virt_base, entry->addr,
> -                                (entry->addr + entry->u.mem.size));
> +                error_report("%s: FIFO at %"PRIx64
> +                             " collides with RAM region at %"PRIx64
> +                             "-%"PRIx64, __func__,
> +                             virt_base, entry->addr,
> +                             (entry->addr + entry->u.mem.size));

I wonder why use () here, anyway:

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>                  exit(-1);
>              }
>  
> @@ -284,9 +285,9 @@ void soc_dma_port_add_fifo(struct soc_dma_s *soc, hwaddr 
> virt_base,
>              while (entry < dma->memmap + dma->memmap_size &&
>                              entry->addr <= virt_base) {
>                  if (entry->addr == virt_base && entry->u.fifo.out == out) {
> -                    fprintf(stderr, "%s: FIFO at %"PRIx64
> -                                    " collides FIFO at %"PRIx64 "\n",
> -                                    __func__, virt_base, entry->addr);
> +                    error_report("%s: FIFO at %"PRIx64
> +                                 " collides FIFO at %"PRIx64,
> +                                 __func__, virt_base, entry->addr);
>                      exit(-1);
>                  }
>  
> @@ -321,11 +322,11 @@ void soc_dma_port_add_mem(struct soc_dma_s *soc, 
> uint8_t *phys_base,
>              if ((entry->addr >= virt_base && entry->addr < virt_base + size) 
> ||
>                              (entry->addr <= virt_base &&
>                               entry->addr + entry->u.mem.size > virt_base)) {
> -                fprintf(stderr, "%s: RAM at %"PRIx64 "-%"PRIx64
> -                                " collides with RAM region at %"PRIx64
> -                                "-%"PRIx64 "\n", __func__,
> -                                virt_base, virt_base + size,
> -                                entry->addr, entry->addr + 
> entry->u.mem.size);
> +                error_report("%s: RAM at %"PRIx64 "-%"PRIx64
> +                             " collides with RAM region at %"PRIx64
> +                             "-%"PRIx64, __func__,
> +                             virt_base, virt_base + size,
> +                             entry->addr, entry->addr + entry->u.mem.size);
>                  exit(-1);
>              }
>  
> @@ -334,11 +335,10 @@ void soc_dma_port_add_mem(struct soc_dma_s *soc, 
> uint8_t *phys_base,
>          } else {
>              if (entry->addr >= virt_base &&
>                              entry->addr < virt_base + size) {
> -                fprintf(stderr, "%s: RAM at %"PRIx64 "-%"PRIx64
> -                                " collides with FIFO at %"PRIx64
> -                                "\n", __func__,
> -                                virt_base, virt_base + size,
> -                                entry->addr);
> +                error_report("%s: RAM at %"PRIx64 "-%"PRIx64
> +                             " collides with FIFO at %"PRIx64,
> +                             __func__, virt_base, virt_base + size,
> +                             entry->addr);
>                  exit(-1);
>              }
>  
>