[spice-gtk,v4,12/29] fixup! usb-redir: add files for SCSI and USB MSC implementation

Submitted by Frediano Ziglio on Aug. 27, 2019, 9:22 a.m.

Details

Message ID 20190827092246.10276-13-fziglio@redhat.com
State Superseded
Headers show
Series "added feature of sharing CD image" ( rev: 6 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Aug. 27, 2019, 9:22 a.m.
Do not use G_GUINT32_FORMAT.
We support a minimum of 32 bit architectures.
---
 src/cd-usb-bulk-msd.c | 53 +++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/cd-usb-bulk-msd.c b/src/cd-usb-bulk-msd.c
index 49e01eb6..5d95dac7 100644
--- a/src/cd-usb-bulk-msd.c
+++ b/src/cd-usb-bulk-msd.c
@@ -138,7 +138,7 @@  UsbCdBulkMsdDevice *cd_usb_bulk_msd_alloc(void *usb_user_data, uint32_t max_luns
     cd_usb_bulk_msd_set_state(cd, USB_CD_STATE_INIT);
     cd->usb_user_data = usb_user_data;
 
-    SPICE_DEBUG("Alloc, max_luns:%" G_GUINT32_FORMAT, max_luns);
+    SPICE_DEBUG("Alloc, max_luns:%u", max_luns);
     return cd;
 }
 
@@ -155,7 +155,7 @@  int cd_usb_bulk_msd_realize(UsbCdBulkMsdDevice *cd, uint32_t lun,
 
     rc = cd_scsi_dev_realize(cd->scsi_target, lun, &scsi_dev_params);
     if (rc != 0) {
-        SPICE_ERROR("Failed to realize lun:%" G_GUINT32_FORMAT, lun);
+        SPICE_ERROR("Failed to realize lun:%u", lun);
         return rc;
     }
 
@@ -165,7 +165,7 @@  int cd_usb_bulk_msd_realize(UsbCdBulkMsdDevice *cd, uint32_t lun,
         cd_scsi_dev_request_release(cd->scsi_target, &cd->usb_req.scsi_req);
     }
 
-    SPICE_DEBUG("Realize OK lun:%" G_GUINT32_FORMAT, lun);
+    SPICE_DEBUG("Realize OK lun:%u", lun);
     return 0;
 }
 
@@ -175,11 +175,11 @@  int cd_usb_bulk_msd_lock(UsbCdBulkMsdDevice *cd, uint32_t lun, gboolean lock)
 
     rc = cd_scsi_dev_lock(cd->scsi_target, lun, lock);
     if (rc != 0) {
-        SPICE_ERROR("Failed to lock lun:%" G_GUINT32_FORMAT, lun);
+        SPICE_ERROR("Failed to lock lun:%u", lun);
         return rc;
     }
 
-    SPICE_DEBUG("Lock OK lun:%" G_GUINT32_FORMAT, lun);
+    SPICE_DEBUG("Lock OK lun:%u", lun);
     return 0;
 }
 
@@ -190,11 +190,11 @@  int cd_usb_bulk_msd_load(UsbCdBulkMsdDevice *cd, uint32_t lun,
 
     rc = cd_scsi_dev_load(cd->scsi_target, lun, media_params);
     if (rc != 0) {
-        SPICE_ERROR("Failed to load lun:%" G_GUINT32_FORMAT, lun);
+        SPICE_ERROR("Failed to load lun:%u", lun);
         return rc;
     }
 
-    SPICE_DEBUG("Load OK lun:%" G_GUINT32_FORMAT, lun);
+    SPICE_DEBUG("Load OK lun:%u", lun);
     return 0;
 }
 
@@ -204,7 +204,7 @@  int cd_usb_bulk_msd_get_info(UsbCdBulkMsdDevice *cd, uint32_t lun, CdScsiDeviceI
 
     rc = cd_scsi_dev_get_info(cd->scsi_target, lun, lun_info);
     if (rc != 0) {
-        SPICE_ERROR("Failed to get info lun:%" G_GUINT32_FORMAT, lun);
+        SPICE_ERROR("Failed to get info lun:%u", lun);
         return rc;
     }
 
@@ -217,11 +217,11 @@  int cd_usb_bulk_msd_unload(UsbCdBulkMsdDevice *cd, uint32_t lun)
 
     rc = cd_scsi_dev_unload(cd->scsi_target, lun);
     if (rc != 0) {
-        SPICE_ERROR("Failed to unload lun:%" G_GUINT32_FORMAT, lun);
+        SPICE_ERROR("Failed to unload lun:%u", lun);
         return rc;
     }
 
-    SPICE_DEBUG("Unload OK lun:%" G_GUINT32_FORMAT, lun);
+    SPICE_DEBUG("Unload OK lun:%u", lun);
     return 0;
 }
 
@@ -231,11 +231,11 @@  int cd_usb_bulk_msd_unrealize(UsbCdBulkMsdDevice *cd, uint32_t lun)
 
     rc = cd_scsi_dev_unrealize(cd->scsi_target, lun);
     if (rc != 0) {
-        SPICE_ERROR("Unrealize lun:%" G_GUINT32_FORMAT, lun);
+        SPICE_ERROR("Unrealize lun:%u", lun);
         return rc;
     }
 
-    SPICE_DEBUG("Unrealize lun:%" G_GUINT32_FORMAT, lun);
+    SPICE_DEBUG("Unrealize lun:%u", lun);
     return 0;
 }
 
@@ -264,7 +264,7 @@  static int parse_usb_msd_cmd(UsbCdBulkMsdDevice *cd, uint8_t *buf, uint32_t cbw_
     CdScsiRequest *scsi_req = &usb_req->scsi_req;
 
     if (cbw_len != sizeof(*cbw)) {
-        SPICE_ERROR("CMD: Bad CBW size:%" G_GUINT32_FORMAT, cbw_len);
+        SPICE_ERROR("CMD: Bad CBW size:%u", cbw_len);
         return -1;
     }
     if (le32toh(cbw->sig) != 0x43425355) { /* MSD command signature */
@@ -304,10 +304,10 @@  static int parse_usb_msd_cmd(UsbCdBulkMsdDevice *cd, uint8_t *buf, uint32_t cbw_
 
     scsi_req->lun = usb_req->lun;
 
-    SPICE_DEBUG("CMD lun:%" G_GUINT32_FORMAT " tag:%#x flags:%08x "
-        "cdb_len:%" G_GUINT32_FORMAT " req_len:%" G_GUINT32_FORMAT,
-        usb_req->lun, le32toh(cbw->tag), cbw->flags,
-        scsi_req->cdb_len, usb_req->usb_req_len);
+    SPICE_DEBUG("CMD lun:%u tag:%#x flags:%08x "
+                "cdb_len:%u req_len:%u",
+                usb_req->lun, le32toh(cbw->tag), cbw->flags,
+                scsi_req->cdb_len, usb_req->usb_req_len);
 
     /* prepare status - CSW */
     usb_req->csw.sig = htole32(0x53425355);
@@ -364,8 +364,8 @@  static void usb_cd_send_data_in(UsbCdBulkMsdDevice *cd, uint32_t max_len)
     uint32_t avail_len = usb_req->scsi_in_len - usb_req->xfer_len;
     uint32_t send_len = MIN(avail_len, max_len);
 
-    SPICE_DEBUG("Data-in cmd tag 0x%x, remains %" G_GUINT32_FORMAT
-                ", requested %" G_GUINT32_FORMAT ", send %" G_GUINT32_FORMAT,
+    SPICE_DEBUG("Data-in cmd tag 0x%x, remains %u"
+                ", requested %u, send %u",
                 usb_req->csw.tag, avail_len, max_len, send_len);
 
     g_assert(max_len <= usb_req->usb_req_len);
@@ -396,7 +396,7 @@  int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd, uint32_t max_len)
     UsbCdBulkMsdRequest *usb_req = &cd->usb_req;
     CdScsiRequest *scsi_req = &usb_req->scsi_req;
 
-    SPICE_DEBUG("msd_read, state: %s, len %" G_GUINT32_FORMAT,
+    SPICE_DEBUG("msd_read, state: %s, len %u",
                 usb_cd_state_str(cd->state), max_len);
 
     switch (cd->state) {
@@ -408,8 +408,8 @@  int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd, uint32_t max_len)
             usb_cd_send_status(cd);
         } else {
             usb_req->bulk_in_len += max_len;
-            SPICE_DEBUG("msd_read CSW, req incomplete, added len %" G_GUINT32_FORMAT
-                        " saved len %" G_GUINT32_FORMAT,
+            SPICE_DEBUG("msd_read CSW, req incomplete, added len %u"
+                        " saved len %u",
                         max_len, usb_req->bulk_in_len);
         }
         break;
@@ -419,8 +419,7 @@  int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd, uint32_t max_len)
             usb_cd_send_data_in(cd, max_len);
         } else {
             usb_req->bulk_in_len += max_len;
-            SPICE_DEBUG("msd_read DATAIN, req incomplete, added len %" G_GUINT32_FORMAT
-                        " saved len %" G_GUINT32_FORMAT,
+            SPICE_DEBUG("msd_read DATAIN, req incomplete, added len %u saved len %u",
                         max_len, usb_req->bulk_in_len);
         }
         break;
@@ -433,7 +432,7 @@  int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd, uint32_t max_len)
         break;
 
     default:
-        SPICE_ERROR("Unexpected read state: %s, len %" G_GUINT32_FORMAT,
+        SPICE_ERROR("Unexpected read state: %s, len %u",
                     usb_cd_state_str(cd->state), max_len);
         goto fail;
     }
@@ -507,7 +506,7 @@  int cd_usb_bulk_msd_write(UsbCdBulkMsdDevice *cd, uint8_t *buf_out, uint32_t buf
         cd_usb_bulk_msd_set_state(cd, USB_CD_STATE_CSW); /* Status next */
         break;
     default:
-        SPICE_DEBUG("Unexpected write state: %s, len %" G_GUINT32_FORMAT,
+        SPICE_DEBUG("Unexpected write state: %s, len %u",
                     usb_cd_state_str(cd->state), buf_out_len);
         goto fail;
     }
@@ -536,7 +535,7 @@  void cd_scsi_target_reset_complete(void *target_user_data)
 void cd_scsi_dev_changed(void *target_user_data, uint32_t lun)
 {
     UsbCdBulkMsdDevice *cd = (UsbCdBulkMsdDevice *)target_user_data;
-    SPICE_DEBUG("Device changed, state: %s lun: %" G_GUINT32_FORMAT,
+    SPICE_DEBUG("Device changed, state: %s lun: %u",
                 usb_cd_state_str(cd->state), lun);
     cd_usb_bulk_msd_lun_changed(cd->usb_user_data, lun);
 }

Comments


Frediano Ziglio writes:

> Do not use G_GUINT32_FORMAT.
> We support a minimum of 32 bit architectures.

What was wrong with the old code?

If you want to get rid of G_ macro dependencies, why not use PRIu32?

> ---
>  src/cd-usb-bulk-msd.c | 53 +++++++++++++++++++++----------------------
>  1 file changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/src/cd-usb-bulk-msd.c b/src/cd-usb-bulk-msd.c
> index 49e01eb6..5d95dac7 100644
> --- a/src/cd-usb-bulk-msd.c
> +++ b/src/cd-usb-bulk-msd.c
> @@ -138,7 +138,7 @@ UsbCdBulkMsdDevice *cd_usb_bulk_msd_alloc(void *usb_user_data, uint32_t max_luns
>      cd_usb_bulk_msd_set_state(cd, USB_CD_STATE_INIT);
>      cd->usb_user_data = usb_user_data;
>
> -    SPICE_DEBUG("Alloc, max_luns:%" G_GUINT32_FORMAT, max_luns);
> +    SPICE_DEBUG("Alloc, max_luns:%u", max_luns);
>      return cd;
>  }
>
> @@ -155,7 +155,7 @@ int cd_usb_bulk_msd_realize(UsbCdBulkMsdDevice *cd, uint32_t lun,
>
>      rc = cd_scsi_dev_realize(cd->scsi_target, lun, &scsi_dev_params);
>      if (rc != 0) {
> -        SPICE_ERROR("Failed to realize lun:%" G_GUINT32_FORMAT, lun);
> +        SPICE_ERROR("Failed to realize lun:%u", lun);
>          return rc;
>      }
>
> @@ -165,7 +165,7 @@ int cd_usb_bulk_msd_realize(UsbCdBulkMsdDevice *cd, uint32_t lun,
>          cd_scsi_dev_request_release(cd->scsi_target, &cd->usb_req.scsi_req);
>      }
>
> -    SPICE_DEBUG("Realize OK lun:%" G_GUINT32_FORMAT, lun);
> +    SPICE_DEBUG("Realize OK lun:%u", lun);
>      return 0;
>  }
>
> @@ -175,11 +175,11 @@ int cd_usb_bulk_msd_lock(UsbCdBulkMsdDevice *cd, uint32_t lun, gboolean lock)
>
>      rc = cd_scsi_dev_lock(cd->scsi_target, lun, lock);
>      if (rc != 0) {
> -        SPICE_ERROR("Failed to lock lun:%" G_GUINT32_FORMAT, lun);
> +        SPICE_ERROR("Failed to lock lun:%u", lun);
>          return rc;
>      }
>
> -    SPICE_DEBUG("Lock OK lun:%" G_GUINT32_FORMAT, lun);
> +    SPICE_DEBUG("Lock OK lun:%u", lun);
>      return 0;
>  }
>
> @@ -190,11 +190,11 @@ int cd_usb_bulk_msd_load(UsbCdBulkMsdDevice *cd, uint32_t lun,
>
>      rc = cd_scsi_dev_load(cd->scsi_target, lun, media_params);
>      if (rc != 0) {
> -        SPICE_ERROR("Failed to load lun:%" G_GUINT32_FORMAT, lun);
> +        SPICE_ERROR("Failed to load lun:%u", lun);
>          return rc;
>      }
>
> -    SPICE_DEBUG("Load OK lun:%" G_GUINT32_FORMAT, lun);
> +    SPICE_DEBUG("Load OK lun:%u", lun);
>      return 0;
>  }
>
> @@ -204,7 +204,7 @@ int cd_usb_bulk_msd_get_info(UsbCdBulkMsdDevice *cd, uint32_t lun, CdScsiDeviceI
>
>      rc = cd_scsi_dev_get_info(cd->scsi_target, lun, lun_info);
>      if (rc != 0) {
> -        SPICE_ERROR("Failed to get info lun:%" G_GUINT32_FORMAT, lun);
> +        SPICE_ERROR("Failed to get info lun:%u", lun);
>          return rc;
>      }
>
> @@ -217,11 +217,11 @@ int cd_usb_bulk_msd_unload(UsbCdBulkMsdDevice *cd, uint32_t lun)
>
>      rc = cd_scsi_dev_unload(cd->scsi_target, lun);
>      if (rc != 0) {
> -        SPICE_ERROR("Failed to unload lun:%" G_GUINT32_FORMAT, lun);
> +        SPICE_ERROR("Failed to unload lun:%u", lun);
>          return rc;
>      }
>
> -    SPICE_DEBUG("Unload OK lun:%" G_GUINT32_FORMAT, lun);
> +    SPICE_DEBUG("Unload OK lun:%u", lun);
>      return 0;
>  }
>
> @@ -231,11 +231,11 @@ int cd_usb_bulk_msd_unrealize(UsbCdBulkMsdDevice *cd, uint32_t lun)
>
>      rc = cd_scsi_dev_unrealize(cd->scsi_target, lun);
>      if (rc != 0) {
> -        SPICE_ERROR("Unrealize lun:%" G_GUINT32_FORMAT, lun);
> +        SPICE_ERROR("Unrealize lun:%u", lun);
>          return rc;
>      }
>
> -    SPICE_DEBUG("Unrealize lun:%" G_GUINT32_FORMAT, lun);
> +    SPICE_DEBUG("Unrealize lun:%u", lun);
>      return 0;
>  }
>
> @@ -264,7 +264,7 @@ static int parse_usb_msd_cmd(UsbCdBulkMsdDevice *cd, uint8_t *buf, uint32_t cbw_
>      CdScsiRequest *scsi_req = &usb_req->scsi_req;
>
>      if (cbw_len != sizeof(*cbw)) {
> -        SPICE_ERROR("CMD: Bad CBW size:%" G_GUINT32_FORMAT, cbw_len);
> +        SPICE_ERROR("CMD: Bad CBW size:%u", cbw_len);
>          return -1;
>      }
>      if (le32toh(cbw->sig) != 0x43425355) { /* MSD command signature */
> @@ -304,10 +304,10 @@ static int parse_usb_msd_cmd(UsbCdBulkMsdDevice *cd, uint8_t *buf, uint32_t cbw_
>
>      scsi_req->lun = usb_req->lun;
>
> -    SPICE_DEBUG("CMD lun:%" G_GUINT32_FORMAT " tag:%#x flags:%08x "
> -        "cdb_len:%" G_GUINT32_FORMAT " req_len:%" G_GUINT32_FORMAT,
> -        usb_req->lun, le32toh(cbw->tag), cbw->flags,
> -        scsi_req->cdb_len, usb_req->usb_req_len);
> +    SPICE_DEBUG("CMD lun:%u tag:%#x flags:%08x "
> +                "cdb_len:%u req_len:%u",
> +                usb_req->lun, le32toh(cbw->tag), cbw->flags,
> +                scsi_req->cdb_len, usb_req->usb_req_len);
>
>      /* prepare status - CSW */
>      usb_req->csw.sig = htole32(0x53425355);
> @@ -364,8 +364,8 @@ static void usb_cd_send_data_in(UsbCdBulkMsdDevice *cd, uint32_t max_len)
>      uint32_t avail_len = usb_req->scsi_in_len - usb_req->xfer_len;
>      uint32_t send_len = MIN(avail_len, max_len);
>
> -    SPICE_DEBUG("Data-in cmd tag 0x%x, remains %" G_GUINT32_FORMAT
> -                ", requested %" G_GUINT32_FORMAT ", send %" G_GUINT32_FORMAT,
> +    SPICE_DEBUG("Data-in cmd tag 0x%x, remains %u"
> +                ", requested %u, send %u",
>                  usb_req->csw.tag, avail_len, max_len, send_len);
>
>      g_assert(max_len <= usb_req->usb_req_len);
> @@ -396,7 +396,7 @@ int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd, uint32_t max_len)
>      UsbCdBulkMsdRequest *usb_req = &cd->usb_req;
>      CdScsiRequest *scsi_req = &usb_req->scsi_req;
>
> -    SPICE_DEBUG("msd_read, state: %s, len %" G_GUINT32_FORMAT,
> +    SPICE_DEBUG("msd_read, state: %s, len %u",
>                  usb_cd_state_str(cd->state), max_len);
>
>      switch (cd->state) {
> @@ -408,8 +408,8 @@ int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd, uint32_t max_len)
>              usb_cd_send_status(cd);
>          } else {
>              usb_req->bulk_in_len += max_len;
> -            SPICE_DEBUG("msd_read CSW, req incomplete, added len %" G_GUINT32_FORMAT
> -                        " saved len %" G_GUINT32_FORMAT,
> +            SPICE_DEBUG("msd_read CSW, req incomplete, added len %u"
> +                        " saved len %u",
>                          max_len, usb_req->bulk_in_len);
>          }
>          break;
> @@ -419,8 +419,7 @@ int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd, uint32_t max_len)
>              usb_cd_send_data_in(cd, max_len);
>          } else {
>              usb_req->bulk_in_len += max_len;
> -            SPICE_DEBUG("msd_read DATAIN, req incomplete, added len %" G_GUINT32_FORMAT
> -                        " saved len %" G_GUINT32_FORMAT,
> +            SPICE_DEBUG("msd_read DATAIN, req incomplete, added len %u saved len %u",
>                          max_len, usb_req->bulk_in_len);
>          }
>          break;
> @@ -433,7 +432,7 @@ int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd, uint32_t max_len)
>          break;
>
>      default:
> -        SPICE_ERROR("Unexpected read state: %s, len %" G_GUINT32_FORMAT,
> +        SPICE_ERROR("Unexpected read state: %s, len %u",
>                      usb_cd_state_str(cd->state), max_len);
>          goto fail;
>      }
> @@ -507,7 +506,7 @@ int cd_usb_bulk_msd_write(UsbCdBulkMsdDevice *cd, uint8_t *buf_out, uint32_t buf
>          cd_usb_bulk_msd_set_state(cd, USB_CD_STATE_CSW); /* Status next */
>          break;
>      default:
> -        SPICE_DEBUG("Unexpected write state: %s, len %" G_GUINT32_FORMAT,
> +        SPICE_DEBUG("Unexpected write state: %s, len %u",
>                      usb_cd_state_str(cd->state), buf_out_len);
>          goto fail;
>      }
> @@ -536,7 +535,7 @@ void cd_scsi_target_reset_complete(void *target_user_data)
>  void cd_scsi_dev_changed(void *target_user_data, uint32_t lun)
>  {
>      UsbCdBulkMsdDevice *cd = (UsbCdBulkMsdDevice *)target_user_data;
> -    SPICE_DEBUG("Device changed, state: %s lun: %" G_GUINT32_FORMAT,
> +    SPICE_DEBUG("Device changed, state: %s lun: %u",
>                  usb_cd_state_str(cd->state), lun);
>      cd_usb_bulk_msd_lun_changed(cd->usb_user_data, lun);
>  }


--
Cheers,
Christophe de Dinechin (IRC c3d)
> Frediano Ziglio writes:
> 
> > Do not use G_GUINT32_FORMAT.
> > We support a minimum of 32 bit architectures.
> 
> What was wrong with the old code?
> 

Shorter and easier to read. I mean '%u' and '%" G_GUINT32_FORMAT "',
if you have an issue is harder to get it.
For instance it was hard to spot the misleading '0x%" G_GUINT32_FORMAT "'
which got '0x%u' so 100 would be formatted as 0x100 which I would read
as 256, not 100.

> If you want to get rid of G_ macro dependencies, why not use PRIu32?
> 

No, G_ macros are fine, also taking into account that PRIu64 and G_GUINT64_FORMAT
could be different (for instance in Windows) and that the receiving formatting
function accepts better the G_ version (they are GLib).

> > ---
> >  src/cd-usb-bulk-msd.c | 53 +++++++++++++++++++++----------------------
> >  1 file changed, 26 insertions(+), 27 deletions(-)
> >
> > diff --git a/src/cd-usb-bulk-msd.c b/src/cd-usb-bulk-msd.c
> > index 49e01eb6..5d95dac7 100644
> > --- a/src/cd-usb-bulk-msd.c
> > +++ b/src/cd-usb-bulk-msd.c
> > @@ -138,7 +138,7 @@ UsbCdBulkMsdDevice *cd_usb_bulk_msd_alloc(void
> > *usb_user_data, uint32_t max_luns
> >      cd_usb_bulk_msd_set_state(cd, USB_CD_STATE_INIT);
> >      cd->usb_user_data = usb_user_data;
> >
> > -    SPICE_DEBUG("Alloc, max_luns:%" G_GUINT32_FORMAT, max_luns);
> > +    SPICE_DEBUG("Alloc, max_luns:%u", max_luns);
> >      return cd;
> >  }
> >
> > @@ -155,7 +155,7 @@ int cd_usb_bulk_msd_realize(UsbCdBulkMsdDevice *cd,
> > uint32_t lun,
> >
> >      rc = cd_scsi_dev_realize(cd->scsi_target, lun, &scsi_dev_params);
> >      if (rc != 0) {
> > -        SPICE_ERROR("Failed to realize lun:%" G_GUINT32_FORMAT, lun);
> > +        SPICE_ERROR("Failed to realize lun:%u", lun);
> >          return rc;
> >      }
> >
> > @@ -165,7 +165,7 @@ int cd_usb_bulk_msd_realize(UsbCdBulkMsdDevice *cd,
> > uint32_t lun,
> >          cd_scsi_dev_request_release(cd->scsi_target,
> >          &cd->usb_req.scsi_req);
> >      }
> >
> > -    SPICE_DEBUG("Realize OK lun:%" G_GUINT32_FORMAT, lun);
> > +    SPICE_DEBUG("Realize OK lun:%u", lun);
> >      return 0;
> >  }
> >
> > @@ -175,11 +175,11 @@ int cd_usb_bulk_msd_lock(UsbCdBulkMsdDevice *cd,
> > uint32_t lun, gboolean lock)
> >
> >      rc = cd_scsi_dev_lock(cd->scsi_target, lun, lock);
> >      if (rc != 0) {
> > -        SPICE_ERROR("Failed to lock lun:%" G_GUINT32_FORMAT, lun);
> > +        SPICE_ERROR("Failed to lock lun:%u", lun);
> >          return rc;
> >      }
> >
> > -    SPICE_DEBUG("Lock OK lun:%" G_GUINT32_FORMAT, lun);
> > +    SPICE_DEBUG("Lock OK lun:%u", lun);
> >      return 0;
> >  }
> >
> > @@ -190,11 +190,11 @@ int cd_usb_bulk_msd_load(UsbCdBulkMsdDevice *cd,
> > uint32_t lun,
> >
> >      rc = cd_scsi_dev_load(cd->scsi_target, lun, media_params);
> >      if (rc != 0) {
> > -        SPICE_ERROR("Failed to load lun:%" G_GUINT32_FORMAT, lun);
> > +        SPICE_ERROR("Failed to load lun:%u", lun);
> >          return rc;
> >      }
> >
> > -    SPICE_DEBUG("Load OK lun:%" G_GUINT32_FORMAT, lun);
> > +    SPICE_DEBUG("Load OK lun:%u", lun);
> >      return 0;
> >  }
> >
> > @@ -204,7 +204,7 @@ int cd_usb_bulk_msd_get_info(UsbCdBulkMsdDevice *cd,
> > uint32_t lun, CdScsiDeviceI
> >
> >      rc = cd_scsi_dev_get_info(cd->scsi_target, lun, lun_info);
> >      if (rc != 0) {
> > -        SPICE_ERROR("Failed to get info lun:%" G_GUINT32_FORMAT, lun);
> > +        SPICE_ERROR("Failed to get info lun:%u", lun);
> >          return rc;
> >      }
> >
> > @@ -217,11 +217,11 @@ int cd_usb_bulk_msd_unload(UsbCdBulkMsdDevice *cd,
> > uint32_t lun)
> >
> >      rc = cd_scsi_dev_unload(cd->scsi_target, lun);
> >      if (rc != 0) {
> > -        SPICE_ERROR("Failed to unload lun:%" G_GUINT32_FORMAT, lun);
> > +        SPICE_ERROR("Failed to unload lun:%u", lun);
> >          return rc;
> >      }
> >
> > -    SPICE_DEBUG("Unload OK lun:%" G_GUINT32_FORMAT, lun);
> > +    SPICE_DEBUG("Unload OK lun:%u", lun);
> >      return 0;
> >  }
> >
> > @@ -231,11 +231,11 @@ int cd_usb_bulk_msd_unrealize(UsbCdBulkMsdDevice *cd,
> > uint32_t lun)
> >
> >      rc = cd_scsi_dev_unrealize(cd->scsi_target, lun);
> >      if (rc != 0) {
> > -        SPICE_ERROR("Unrealize lun:%" G_GUINT32_FORMAT, lun);
> > +        SPICE_ERROR("Unrealize lun:%u", lun);
> >          return rc;
> >      }
> >
> > -    SPICE_DEBUG("Unrealize lun:%" G_GUINT32_FORMAT, lun);
> > +    SPICE_DEBUG("Unrealize lun:%u", lun);
> >      return 0;
> >  }
> >
> > @@ -264,7 +264,7 @@ static int parse_usb_msd_cmd(UsbCdBulkMsdDevice *cd,
> > uint8_t *buf, uint32_t cbw_
> >      CdScsiRequest *scsi_req = &usb_req->scsi_req;
> >
> >      if (cbw_len != sizeof(*cbw)) {
> > -        SPICE_ERROR("CMD: Bad CBW size:%" G_GUINT32_FORMAT, cbw_len);
> > +        SPICE_ERROR("CMD: Bad CBW size:%u", cbw_len);
> >          return -1;
> >      }
> >      if (le32toh(cbw->sig) != 0x43425355) { /* MSD command signature */
> > @@ -304,10 +304,10 @@ static int parse_usb_msd_cmd(UsbCdBulkMsdDevice *cd,
> > uint8_t *buf, uint32_t cbw_
> >
> >      scsi_req->lun = usb_req->lun;
> >
> > -    SPICE_DEBUG("CMD lun:%" G_GUINT32_FORMAT " tag:%#x flags:%08x "
> > -        "cdb_len:%" G_GUINT32_FORMAT " req_len:%" G_GUINT32_FORMAT,
> > -        usb_req->lun, le32toh(cbw->tag), cbw->flags,
> > -        scsi_req->cdb_len, usb_req->usb_req_len);
> > +    SPICE_DEBUG("CMD lun:%u tag:%#x flags:%08x "
> > +                "cdb_len:%u req_len:%u",
> > +                usb_req->lun, le32toh(cbw->tag), cbw->flags,
> > +                scsi_req->cdb_len, usb_req->usb_req_len);
> >
> >      /* prepare status - CSW */
> >      usb_req->csw.sig = htole32(0x53425355);
> > @@ -364,8 +364,8 @@ static void usb_cd_send_data_in(UsbCdBulkMsdDevice *cd,
> > uint32_t max_len)
> >      uint32_t avail_len = usb_req->scsi_in_len - usb_req->xfer_len;
> >      uint32_t send_len = MIN(avail_len, max_len);
> >
> > -    SPICE_DEBUG("Data-in cmd tag 0x%x, remains %" G_GUINT32_FORMAT
> > -                ", requested %" G_GUINT32_FORMAT ", send %"
> > G_GUINT32_FORMAT,
> > +    SPICE_DEBUG("Data-in cmd tag 0x%x, remains %u"
> > +                ", requested %u, send %u",
> >                  usb_req->csw.tag, avail_len, max_len, send_len);
> >
> >      g_assert(max_len <= usb_req->usb_req_len);
> > @@ -396,7 +396,7 @@ int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd,
> > uint32_t max_len)
> >      UsbCdBulkMsdRequest *usb_req = &cd->usb_req;
> >      CdScsiRequest *scsi_req = &usb_req->scsi_req;
> >
> > -    SPICE_DEBUG("msd_read, state: %s, len %" G_GUINT32_FORMAT,
> > +    SPICE_DEBUG("msd_read, state: %s, len %u",
> >                  usb_cd_state_str(cd->state), max_len);
> >
> >      switch (cd->state) {
> > @@ -408,8 +408,8 @@ int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd,
> > uint32_t max_len)
> >              usb_cd_send_status(cd);
> >          } else {
> >              usb_req->bulk_in_len += max_len;
> > -            SPICE_DEBUG("msd_read CSW, req incomplete, added len %"
> > G_GUINT32_FORMAT
> > -                        " saved len %" G_GUINT32_FORMAT,
> > +            SPICE_DEBUG("msd_read CSW, req incomplete, added len %u"
> > +                        " saved len %u",
> >                          max_len, usb_req->bulk_in_len);
> >          }
> >          break;
> > @@ -419,8 +419,7 @@ int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd,
> > uint32_t max_len)
> >              usb_cd_send_data_in(cd, max_len);
> >          } else {
> >              usb_req->bulk_in_len += max_len;
> > -            SPICE_DEBUG("msd_read DATAIN, req incomplete, added len %"
> > G_GUINT32_FORMAT
> > -                        " saved len %" G_GUINT32_FORMAT,
> > +            SPICE_DEBUG("msd_read DATAIN, req incomplete, added len %u
> > saved len %u",
> >                          max_len, usb_req->bulk_in_len);
> >          }
> >          break;
> > @@ -433,7 +432,7 @@ int cd_usb_bulk_msd_read(UsbCdBulkMsdDevice *cd,
> > uint32_t max_len)
> >          break;
> >
> >      default:
> > -        SPICE_ERROR("Unexpected read state: %s, len %" G_GUINT32_FORMAT,
> > +        SPICE_ERROR("Unexpected read state: %s, len %u",
> >                      usb_cd_state_str(cd->state), max_len);
> >          goto fail;
> >      }
> > @@ -507,7 +506,7 @@ int cd_usb_bulk_msd_write(UsbCdBulkMsdDevice *cd,
> > uint8_t *buf_out, uint32_t buf
> >          cd_usb_bulk_msd_set_state(cd, USB_CD_STATE_CSW); /* Status next */
> >          break;
> >      default:
> > -        SPICE_DEBUG("Unexpected write state: %s, len %" G_GUINT32_FORMAT,
> > +        SPICE_DEBUG("Unexpected write state: %s, len %u",
> >                      usb_cd_state_str(cd->state), buf_out_len);
> >          goto fail;
> >      }
> > @@ -536,7 +535,7 @@ void cd_scsi_target_reset_complete(void
> > *target_user_data)
> >  void cd_scsi_dev_changed(void *target_user_data, uint32_t lun)
> >  {
> >      UsbCdBulkMsdDevice *cd = (UsbCdBulkMsdDevice *)target_user_data;
> > -    SPICE_DEBUG("Device changed, state: %s lun: %" G_GUINT32_FORMAT,
> > +    SPICE_DEBUG("Device changed, state: %s lun: %u",
> >                  usb_cd_state_str(cd->state), lun);
> >      cd_usb_bulk_msd_lun_changed(cd->usb_user_data, lun);
> >  }
Frediano Ziglio writes:

>> Frediano Ziglio writes:
>>
>> > Do not use G_GUINT32_FORMAT.
>> > We support a minimum of 32 bit architectures.
>>
>> What was wrong with the old code?
>>
>
> Shorter and easier to read. I mean '%u' and '%" G_GUINT32_FORMAT "',
> if you have an issue is harder to get it.
> For instance it was hard to spot the misleading '0x%" G_GUINT32_FORMAT "'
> which got '0x%u' so 100 would be formatted as 0x100 which I would read
> as 256, not 100.

Ah, I can understand how this could have been hard to find.

Still, even if I now understand where you come from, and even if I know
that your patch does not really hurt in practice given what is currently
known of the support platforms, I still see that patch as going somewhat
in the wrong direction as far as portability best practices are concerned.

Reviewed-but-not-entirely-approved-by: Christophe de Dinechin <dinechin@redhat.com>

>
>> If you want to get rid of G_ macro dependencies, why not use PRIu32?
>>
>
> No, G_ macros are fine, also taking into account that PRIu64 and G_GUINT64_FORMAT
> could be different (for instance in Windows) and that the receiving formatting
> function accepts better the G_ version (they are GLib).

This comment, while it reinforces your rationale, made me extremely sad
;-)


--
Cheers,
Christophe de Dinechin (IRC c3d)
> 
> Frediano Ziglio writes:
> 
> >>
> >> > Do not use G_GUINT32_FORMAT.
> >> > We support a minimum of 32 bit architectures.
> >>
> >> What was wrong with the old code?
> >>
> >
> > Shorter and easier to read. I mean '%u' and '%" G_GUINT32_FORMAT "',
> > if you have an issue is harder to get it.
> > For instance it was hard to spot the misleading '0x%" G_GUINT32_FORMAT "'
> > which got '0x%u' so 100 would be formatted as 0x100 which I would read
> > as 256, not 100.
> 
> Ah, I can understand how this could have been hard to find.
> 
> Still, even if I now understand where you come from, and even if I know
> that your patch does not really hurt in practice given what is currently
> known of the support platforms, I still see that patch as going somewhat
> in the wrong direction as far as portability best practices are concerned.
> 

Do you think we are even going to support 16 bit platforms? This would
require quite a rewrite of the entire project and also of the dependent
libraries. And I don't see a 16 bit program supporting such a large
memory requirements so easily.

> Reviewed-but-not-entirely-approved-by: Christophe de Dinechin
> <dinechin@redhat.com>
> 
> >
> >> If you want to get rid of G_ macro dependencies, why not use PRIu32?
> >>
> >
> > No, G_ macros are fine, also taking into account that PRIu64 and
> > G_GUINT64_FORMAT
> > could be different (for instance in Windows) and that the receiving
> > formatting
> > function accepts better the G_ version (they are GLib).
> 
> This comment, while it reinforces your rationale, made me extremely sad
> ;-)
> 

I did some minor checks yesterday about formatting strings and Windows.
That's a weird mixture of compatibility and changes (like GLib that
changed formatting along the way for MingW or newer msvcrt.dll supporting
C99 style formatting). Giving that Windows XP support was removed and
that Windows 7 seems to support C99 formatting I would say "ll" for
64 bit is good and also "I64" for ABI compatibility. So it looks like
there should be a way to tell GCC that the printf/ms_printf formatting
attribute should accepts also "ll" size modified for 64 bit integrals.

Frediano