[v4,1/5] 9p: unsigned type for type, version, path

There is no need for signedness on these QID fields for 9p.

Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 fsdev/9p-marshal.h   |  6 +++---
 hw/9pfs/9p.c         |  6 +++---
 hw/9pfs/trace-events | 14 +++++++-------
 3 files changed, 13 insertions(+), 13 deletions(-)

On Wed, 26 Jun 2019 20:25:55 +0200
Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org> wrote:

> There is no need for signedness on these QID fields for 9p.
> 
> Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>

You should mention here the changes you made on top of Antonios
original patch. Something like:

[CS: - also convert path
     - adapted trace-events and donttouch_stat()]

> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  fsdev/9p-marshal.h   |  6 +++---
>  hw/9pfs/9p.c         |  6 +++---
>  hw/9pfs/trace-events | 14 +++++++-------
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/fsdev/9p-marshal.h b/fsdev/9p-marshal.h
> index c8823d878f..8f3babb60a 100644
> --- a/fsdev/9p-marshal.h
> +++ b/fsdev/9p-marshal.h
> @@ -9,9 +9,9 @@ typedef struct V9fsString
>  
>  typedef struct V9fsQID
>  {
> -    int8_t type;
> -    int32_t version;
> -    int64_t path;
> +    uint8_t type;
> +    uint32_t version;
> +    uint64_t path;
>  } V9fsQID;
>  
>  typedef struct V9fsStat
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 55821343e5..586a6dccba 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -743,9 +743,9 @@ static int donttouch_stat(V9fsStat *stat)
>  {
>      if (stat->type == -1 &&
>          stat->dev == -1 &&
> -        stat->qid.type == -1 &&
> -        stat->qid.version == -1 &&
> -        stat->qid.path == -1 &&
> +        stat->qid.type == 0xff &&
> +        stat->qid.version == (uint32_t) -1 &&
> +        stat->qid.path == (uint64_t) -1 &&
>          stat->mode == -1 &&
>          stat->atime == -1 &&
>          stat->mtime == -1 &&
> diff --git a/hw/9pfs/trace-events b/hw/9pfs/trace-events
> index c0a0a4ab5d..6964756922 100644
> --- a/hw/9pfs/trace-events
> +++ b/hw/9pfs/trace-events
> @@ -6,7 +6,7 @@ v9fs_rerror(uint16_t tag, uint8_t id, int err) "tag %d id %d err %d"
>  v9fs_version(uint16_t tag, uint8_t id, int32_t msize, char* version) "tag %d id %d msize %d version %s"
>  v9fs_version_return(uint16_t tag, uint8_t id, int32_t msize, char* version) "tag %d id %d msize %d version %s"
>  v9fs_attach(uint16_t tag, uint8_t id, int32_t fid, int32_t afid, char* uname, char* aname) "tag %u id %u fid %d afid %d uname %s aname %s"
> -v9fs_attach_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path) "tag %d id %d type %d version %d path %"PRId64
> +v9fs_attach_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path) "tag %d id %d type %d version %d path %"PRId64

I was expecting to see PRIu64 for an uint64_t but I now realize that %d
seems to be used all over the place for unsigned types... :-\

At least, please fix the masks of the lines you're changing in this
patch so that unsigned are passed to "u" or PRIu64. The rest of the
mess can be fixed later in a followup.

>  v9fs_stat(uint16_t tag, uint8_t id, int32_t fid) "tag %d id %d fid %d"
>  v9fs_stat_return(uint16_t tag, uint8_t id, int32_t mode, int32_t atime, int32_t mtime, int64_t length) "tag %d id %d stat={mode %d atime %d mtime %d length %"PRId64"}"
>  v9fs_getattr(uint16_t tag, uint8_t id, int32_t fid, uint64_t request_mask) "tag %d id %d fid %d request_mask %"PRIu64
> @@ -14,9 +14,9 @@ v9fs_getattr_return(uint16_t tag, uint8_t id, uint64_t result_mask, uint32_t mod
>  v9fs_walk(uint16_t tag, uint8_t id, int32_t fid, int32_t newfid, uint16_t nwnames) "tag %d id %d fid %d newfid %d nwnames %d"
>  v9fs_walk_return(uint16_t tag, uint8_t id, uint16_t nwnames, void* qids) "tag %d id %d nwnames %d qids %p"
>  v9fs_open(uint16_t tag, uint8_t id, int32_t fid, int32_t mode) "tag %d id %d fid %d mode %d"
> -v9fs_open_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
> +v9fs_open_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
>  v9fs_lcreate(uint16_t tag, uint8_t id, int32_t dfid, int32_t flags, int32_t mode, uint32_t gid) "tag %d id %d dfid %d flags %d mode %d gid %u"
> -v9fs_lcreate_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int32_t iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
> +v9fs_lcreate_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int32_t iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
>  v9fs_fsync(uint16_t tag, uint8_t id, int32_t fid, int datasync) "tag %d id %d fid %d datasync %d"
>  v9fs_clunk(uint16_t tag, uint8_t id, int32_t fid) "tag %d id %d fid %d"
>  v9fs_read(uint16_t tag, uint8_t id, int32_t fid, uint64_t off, uint32_t max_count) "tag %d id %d fid %d off %"PRIu64" max_count %u"
> @@ -26,21 +26,21 @@ v9fs_readdir_return(uint16_t tag, uint8_t id, uint32_t count, ssize_t retval) "t
>  v9fs_write(uint16_t tag, uint8_t id, int32_t fid, uint64_t off, uint32_t count, int cnt) "tag %d id %d fid %d off %"PRIu64" count %u cnt %d"
>  v9fs_write_return(uint16_t tag, uint8_t id, int32_t total, ssize_t err) "tag %d id %d total %d err %zd"
>  v9fs_create(uint16_t tag, uint8_t id, int32_t fid, char* name, int32_t perm, int8_t mode) "tag %d id %d fid %d name %s perm %d mode %d"
> -v9fs_create_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
> +v9fs_create_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int iounit) "tag %d id %d qid={type %d version %d path %"PRId64"} iounit %d"
>  v9fs_symlink(uint16_t tag, uint8_t id, int32_t fid,  char* name, char* symname, uint32_t gid) "tag %d id %d fid %d name %s symname %s gid %u"
> -v9fs_symlink_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
> +v9fs_symlink_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
>  v9fs_flush(uint16_t tag, uint8_t id, int16_t flush_tag) "tag %d id %d flush_tag %d"
>  v9fs_link(uint16_t tag, uint8_t id, int32_t dfid, int32_t oldfid, char* name) "tag %d id %d dfid %d oldfid %d name %s"
>  v9fs_remove(uint16_t tag, uint8_t id, int32_t fid) "tag %d id %d fid %d"
>  v9fs_wstat(uint16_t tag, uint8_t id, int32_t fid, int32_t mode, int32_t atime, int32_t mtime) "tag %u id %u fid %d stat={mode %d atime %d mtime %d}"
>  v9fs_mknod(uint16_t tag, uint8_t id, int32_t fid, int mode, int major, int minor) "tag %d id %d fid %d mode %d major %d minor %d"
> -v9fs_mknod_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
> +v9fs_mknod_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path) "tag %d id %d qid={type %d version %d path %"PRId64"}"
>  v9fs_lock(uint16_t tag, uint8_t id, int32_t fid, uint8_t type, uint64_t start, uint64_t length) "tag %d id %d fid %d type %d start %"PRIu64" length %"PRIu64
>  v9fs_lock_return(uint16_t tag, uint8_t id, int8_t status) "tag %d id %d status %d"
>  v9fs_getlock(uint16_t tag, uint8_t id, int32_t fid, uint8_t type, uint64_t start, uint64_t length)"tag %d id %d fid %d type %d start %"PRIu64" length %"PRIu64
>  v9fs_getlock_return(uint16_t tag, uint8_t id, uint8_t type, uint64_t start, uint64_t length, uint32_t proc_id) "tag %d id %d type %d start %"PRIu64" length %"PRIu64" proc_id %u"
>  v9fs_mkdir(uint16_t tag, uint8_t id, int32_t fid, char* name, int mode, uint32_t gid) "tag %u id %u fid %d name %s mode %d gid %u"
> -v9fs_mkdir_return(uint16_t tag, uint8_t id, int8_t type, int32_t version, int64_t path, int err) "tag %u id %u qid={type %d version %d path %"PRId64"} err %d"
> +v9fs_mkdir_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t version, uint64_t path, int err) "tag %u id %u qid={type %d version %d path %"PRId64"} err %d"
>  v9fs_xattrwalk(uint16_t tag, uint8_t id, int32_t fid, int32_t newfid, char* name) "tag %d id %d fid %d newfid %d name %s"
>  v9fs_xattrwalk_return(uint16_t tag, uint8_t id, int64_t size) "tag %d id %d size %"PRId64
>  v9fs_xattrcreate(uint16_t tag, uint8_t id, int32_t fid, char* name, uint64_t size, int flags) "tag %d id %d fid %d name %s size %"PRIu64" flags %d"

On Donnerstag, 27. Juni 2019 18:12:03 CEST Greg Kurz wrote:
> On Wed, 26 Jun 2019 20:25:55 +0200
> Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org> wrote:
> > There is no need for signedness on these QID fields for 9p.
> > 
> > Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
> 
> You should mention here the changes you made on top of Antonios
> original patch. Something like:
> 
> [CS: - also convert path
>      - adapted trace-events and donttouch_stat()]

Haven't seen that comment style in the git logs. Any example hash for that?

> > diff --git a/hw/9pfs/trace-events b/hw/9pfs/trace-events
> > index c0a0a4ab5d..6964756922 100644
> > --- a/hw/9pfs/trace-events
> > +++ b/hw/9pfs/trace-events
> > @@ -6,7 +6,7 @@ v9fs_rerror(uint16_t tag, uint8_t id, int err) "tag %d id
> > %d err %d"> 
> >  v9fs_version(uint16_t tag, uint8_t id, int32_t msize, char* version) "tag
> >  %d id %d msize %d version %s" v9fs_version_return(uint16_t tag, uint8_t
> >  id, int32_t msize, char* version) "tag %d id %d msize %d version %s"
> >  v9fs_attach(uint16_t tag, uint8_t id, int32_t fid, int32_t afid, char*
> >  uname, char* aname) "tag %u id %u fid %d afid %d uname %s aname %s"> 
> > -v9fs_attach_return(uint16_t tag, uint8_t id, int8_t type, int32_t
> > version, int64_t path) "tag %d id %d type %d version %d path %"PRId64
> > +v9fs_attach_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t
> > version, uint64_t path) "tag %d id %d type %d version %d path %"PRId64
> I was expecting to see PRIu64 for an uint64_t but I now realize that %d
> seems to be used all over the place for unsigned types... :-\
> 
> At least, please fix the masks of the lines you're changing in this
> patch so that unsigned are passed to "u" or PRIu64. The rest of the
> mess can be fixed later in a followup.

If you don't mind I will restrict it to your latter suggestion for now, that 
is adjusting it using the short format specifiers e.g. "u", the rest would IMO 
be out of the scope of this patch series.

Too bad that no format specifier warnings are thrown on these.

Best regards,
Christian Schoenebeck

On Fri, 28 Jun 2019 13:42:43 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 27. Juni 2019 18:12:03 CEST Greg Kurz wrote:
> > On Wed, 26 Jun 2019 20:25:55 +0200
> > Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org> wrote:
> > > There is no need for signedness on these QID fields for 9p.
> > > 
> > > Signed-off-by: Antonios Motakis <antonios.motakis@huawei.com>
> > 
> > You should mention here the changes you made on top of Antonios
> > original patch. Something like:
> > 
> > [CS: - also convert path
> >      - adapted trace-events and donttouch_stat()]
> 
> Haven't seen that comment style in the git logs. Any example hash for that?
> 

$ git log | egrep '^[[:space:]]*\[' | head -15
    [Commit message tweaked]
    [Superfluous #include dropped]
    [Comment reformatted to make checkpatch.pl happy, #include <dirent.h>
    [monitor_is_qmp() tidied up to make checkpatch.pl happy,
    [Header guard symbol tidied up, superfluous #include dropped, FIXME in
    [sortcmdlist() cleaned up to make checkpatch.pl happy]
    [Superfluous variable in monitor_data_destroy() eliminated, whitespace
    [Superfluous variable in monitor_data_destroy() eliminated]
    [Zero initialization of Monitor moved from monitor_data_init() to
        [ ... ]
        [ ... ]
    [mreitz: Dropped superfluous printf from _filter_offsets, as suggested
    [mreitz: Adjusted commit message as per John's proposal]
    [mreitz: Moved from 250 to 256]
    [AJB: fix conflicts with tests/vm: Port basevm to Python 3]

This is something you should do when re-posting someone else's patch
with modifications.

> > > diff --git a/hw/9pfs/trace-events b/hw/9pfs/trace-events
> > > index c0a0a4ab5d..6964756922 100644
> > > --- a/hw/9pfs/trace-events
> > > +++ b/hw/9pfs/trace-events
> > > @@ -6,7 +6,7 @@ v9fs_rerror(uint16_t tag, uint8_t id, int err) "tag %d id
> > > %d err %d"> 
> > >  v9fs_version(uint16_t tag, uint8_t id, int32_t msize, char* version) "tag
> > >  %d id %d msize %d version %s" v9fs_version_return(uint16_t tag, uint8_t
> > >  id, int32_t msize, char* version) "tag %d id %d msize %d version %s"
> > >  v9fs_attach(uint16_t tag, uint8_t id, int32_t fid, int32_t afid, char*
> > >  uname, char* aname) "tag %u id %u fid %d afid %d uname %s aname %s"> 
> > > -v9fs_attach_return(uint16_t tag, uint8_t id, int8_t type, int32_t
> > > version, int64_t path) "tag %d id %d type %d version %d path %"PRId64
> > > +v9fs_attach_return(uint16_t tag, uint8_t id, uint8_t type, uint32_t
> > > version, uint64_t path) "tag %d id %d type %d version %d path %"PRId64
> > I was expecting to see PRIu64 for an uint64_t but I now realize that %d
> > seems to be used all over the place for unsigned types... :-\
> > 
> > At least, please fix the masks of the lines you're changing in this
> > patch so that unsigned are passed to "u" or PRIu64. The rest of the
> > mess can be fixed later in a followup.
> 
> If you don't mind I will restrict it to your latter suggestion for now, that 
> is adjusting it using the short format specifiers e.g. "u", the rest would IMO 
> be out of the scope of this patch series.
> 

Sure.

> Too bad that no format specifier warnings are thrown on these.
> 

Yeah :-\

> Best regards,
> Christian Schoenebeck