Re: [Qemu-devel] [PATCH v4 1/5] 9p: unsigned type for type, version, path

[Greg Kurz]

On Fri, 28 Jun 2019 13:42:43 +0200
Christian Schoenebeck <address@hidden> 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 <address@hidden> wrote:
> > > There is no need for signedness on these QID fields for 9p.
> > > 
> > > Signed-off-by: Antonios Motakis <address@hidden>
> > 
> > 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