[Spice-devel] [PATCH spice-server 1/33 v2] Use proper format strings for spice_log

[Frediano Ziglio]

> 
> On Wed, Jan 02, 2019 at 02:56:43PM +0000, Frediano Ziglio wrote:
> > Formatting string should be compatible with GLib.
> > GLib uses formatting types compatible with GNU.
> > For Linux this is not an issue as both systems (like a printf) and
> > GLib one uses the same formatting type.  However on Windows they
> > differs potentially causing issues.
> > This is also make worse as GLib 2.58 changed format attribute from
> > __printf__ to gnu_printf (Microsoft compatibility formats like %I64d
> > are still supported but you'll get warnings using GCC/Clang
> > compilers).
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/char-device.c         |  3 ++-
> >  server/gstreamer-encoder.c   |  4 ++--
> >  server/main-channel-client.c |  6 +++---
> >  server/memslot.c             |  2 +-
> >  server/mjpeg-encoder.c       |  6 ++++--
> >  server/red-channel.c         |  6 ++++--
> >  server/red-client.c          |  6 ++++--
> >  server/red-replay-qxl.c      | 11 ++++++-----
> >  server/reds.c                |  4 ++--
> >  subprojects/spice-common     |  2 +-
> >  10 files changed, 29 insertions(+), 21 deletions(-)
> > 
> > Changes since v1:
> > - update due to GLib changes and compatibility to use GLib format
> >   types.
> > 

...

> > diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> > index 6958a495..4884e97e 100644
> > --- a/server/red-replay-qxl.c
> > +++ b/server/red-replay-qxl.c
> > @@ -227,7 +227,7 @@ static replay_t read_binary(SpiceReplay *replay, const
> > char *prefix, size_t *siz
> >      uint8_t *zlib_buffer;
> >      z_stream strm;
> >  
> > -    snprintf(template, sizeof(template), "binary %%d %s %%ld:%%n",
> > prefix);
> > +    snprintf(template, sizeof(template), "binary %%d %s %%" PRIdPTR
> > ":%%n", prefix);
> 
> This one introduces the use of PRIdPTR, might be worth moving it to a
> separate commit.
> 

Done

> >      replay_fscanf_check(replay, template, &with_zlib, size,
> >      &replay->end_pos);
> >      if (replay->error) {
> >          return REPLAY_ERROR;
> > @@ -266,7 +266,8 @@ static replay_t read_binary(SpiceReplay *replay, const
> > char *prefix, size_t *siz
> >              exit(1);
> >          }
> >          if ((ret = inflate(&strm, Z_NO_FLUSH)) != Z_STREAM_END) {
> > -            spice_error("inflate error %d (disc: %ld)", ret, *size -
> > strm.total_out);
> > +            spice_error("inflate error %d (disc: %" G_GSSIZE_FORMAT ")",
> > +                        ret, *size - strm.total_out);
> >              if (ret == Z_DATA_ERROR) {
> >                  /* last operation may be wrong. since we do the recording
> >                   * in red_worker, when there is a shutdown from the
> >                   vcpu/io thread
> > @@ -475,7 +476,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
> > uint32_t flags)
> >          } else {
> >              size = red_replay_data_chunks(replay, "bitmap.data",
> >              (uint8_t**)&qxl->bitmap.data, 0);
> >              if (size != bitmap_size) {
> > -                g_warning("bad image, %" PRIuPTR " != %" PRIuPTR, size,
> > bitmap_size);
> > +                g_warning("bad image, %" G_GSIZE_FORMAT " != %"
> > G_GSIZE_FORMAT, size, bitmap_size);
> >                  return NULL;
> >              }
> >          }
> > @@ -1137,7 +1138,7 @@ static QXLSurfaceCmd
> > *red_replay_surface_cmd(SpiceReplay *replay)
> >          if ((qxl->flags & QXL_SURF_FLAG_KEEP_DATA) != 0) {
> >              read_binary(replay, "data", &read_size,
> >              (uint8_t**)&qxl->u.surface_create.data, 0);
> >              if (read_size != size) {
> > -                g_warning("mismatch %" PRIuPTR " != %" PRIuPTR, size,
> > read_size);
> > +                g_warning("mismatch %" G_GSIZE_FORMAT " != %"
> > G_GSIZE_FORMAT, size, read_size);
> >              }
> >          } else {
> >              qxl->u.surface_create.data =
> >              QXLPHYSICAL_FROM_PTR(replay_malloc(replay, size));
> > @@ -1321,7 +1322,7 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> > spice_replay_next_cmd(SpiceReplay *replay,
> >      cmd = replay_malloc0(replay, sizeof(QXLCommandExt));
> >      cmd->cmd.type = type;
> >      cmd->group_id = 0;
> > -    spice_debug("command %"SCNu64", %d", timestamp, cmd->cmd.type);
> > +    spice_debug("command %"G_GUINT64_FORMAT", %d", timestamp,
> > cmd->cmd.type);
> >      switch (cmd->cmd.type) {
> >      case QXL_CMD_DRAW:
> >          cmd->flags = 0;
> > diff --git a/server/reds.c b/server/reds.c
> > index cdbb94cb..22198926 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -1149,7 +1149,7 @@ static void
> > reds_on_main_agent_monitors_config(RedsState *reds,
> >      }
> >      spice_buffer_append(cmc, message, size);
> >      if (sizeof(VDAgentMessage) > cmc->offset) {
> > -        spice_debug("not enough data yet. %zd", cmc->offset);
> > +        spice_debug("not enough data yet. %" G_GSSIZE_FORMAT,
> > cmc->offset);
> >          return;
> >      }
> >      msg_header = (VDAgentMessage *)cmc->buffer;
> > @@ -1157,7 +1157,7 @@ static void
> > reds_on_main_agent_monitors_config(RedsState *reds,
> >          goto overflow;
> >      }
> >      if (msg_header->size > cmc->offset - sizeof(VDAgentMessage)) {
> > -        spice_debug("not enough data yet. %zd", cmc->offset);
> > +        spice_debug("not enough data yet. %" G_GSSIZE_FORMAT,
> > cmc->offset);
> >          return;
> >      }
> >      if (msg_header->size < sizeof(VDAgentMonitorsConfig)) {
> > diff --git a/subprojects/spice-common b/subprojects/spice-common
> > index 125cfa7f..6dc745e8 160000
> > --- a/subprojects/spice-common
> > +++ b/subprojects/spice-common
> > @@ -1 +1 @@
> > -Subproject commit 125cfa7fb2493060419c79eae3b40dbc499bd3db
> > +Subproject commit 6dc745e877a12ed25634040dd6fa46fa5de694e2
> 
> Intentional?
> 

Yes, contains a patch for spice_log formatting attribute

> Apart from this,
> 
> Reviewed-by: Christophe Fergeau <cfergeau at redhat.com>
> 
> Christophe
> 

Frediano