* Dmitry V. Levin (l...@altlinux.org) wrote:
> On Thu, Apr 21, 2016 at 02:04:33PM +0100, Dr. David Alan Gilbert wrote:
> [...]
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -420,6 +420,9 @@ AC_CHECK_HEADERS([linux/input.h], [
> >     AC_CHECK_MEMBERS([struct input_absinfo.resolution],,, [#include 
> > <linux/input.h>])
> >  ])
> >  
> > +AC_CHECK_HEADERS([linux/userfaultfd.h], [
> > +])
> 
> This should rather go to the first (big) AC_CHECK_HEADERS list
> in the file.

Done.

> > --- a/ioctl.c
> > +++ b/ioctl.c
> > @@ -263,6 +263,8 @@ ioctl_decode(struct tcb *tcp)
> >     case 'E':
> >             return evdev_ioctl(tcp, code, arg);
> >  #endif
> > +   case 0xaa:
> > +           return uffdio_ioctl(tcp, code, arg);
> 
> The tradition approach is to wrap this into
> #ifdef HAVE_LINUX_USERFAULTFD_H / #endif
> so that ...

Done

> 
> > --- a/userfaultfd.c
> > +++ b/userfaultfd.c
> > @@ -27,9 +27,50 @@
> >  
> >  #include "defs.h"
> >  #include <fcntl.h>
> > +#include <linux/ioctl.h>
> > +#ifdef HAVE_LINUX_USERFAULTFD_H
> > +#include <linux/userfaultfd.h>
> > +#endif
> >  
> >  #include "xlat/uffd_flags.h"
> >  
> > +
> > +int
> > +uffdio_ioctl(struct tcb *tcp, const unsigned int code, const long arg)
> > +{
> > +#ifdef HAVE_LINUX_USERFAULTFD_H
> 
> ... the whole parser with all headers it needs could be wrapped
> with a single ifdef.

OK, that is easier.

> > +   case UFFDIO_COPY: {
> > +           struct uffdio_copy uc;
> > +           if (!umove_or_printaddr(tcp, arg, &uc)) {
> > +                   if (entering(tcp)) {
> > +                           tprintf("{dst=%" PRIx64 ", src=%" PRIx64
> > +                           ", len=%" PRIu64 ", mode=%" PRIu64 "}",
> > +                           (uint64_t)uc.dst, (uint64_t)uc.src,
> > +                           (uint64_t)uc.len, (uint64_t)uc.mode);
> 
> I suppose you can use recently added PRI__*64 instead of PRI*64,
> this way you won't need explicit casts.

Done.

> > +                           return 1;
> > +                   } else {
> > +                           /* copy also returns the number of bytes
> > +                            * copied */
> > +                           sprintf(auxbuf, "copy=%" PRId64,
> > +                           (int64_t)uc.copy);
> > +                           tcp->auxstr = auxbuf;
> > +                           return RVAL_STR | 1;
> > +                   }
> 
> The preferred method is to print uc.copy as a part of structure:
> 
>       if (entering(tcp)) {
>               if (umove_or_printaddr(tcp, arg, &uc))
>                       return RVAL_DECODED | 1;
>               tprintf("{dst=%" PRI__x64 ", src=%" PRI__x64
>                       ", len=%" PRI__u64 ", mode=%" PRI__u64
>                       ", copy=",
>                       uc.dst, uc.src, uc.len, uc.mode);
>                       return 1;
>       } else {
>               if (!umove_or_printaddr(tcp, arg, &uc.copy))
>                       tprintf("%" PRI__d64, uc.copy);
>               tprints("}");
>               return 1;
>       }

OK, thanks, taken that.
strace seems to have PRI__s64 rather than PRId64 that would have matched
the C library; I've also changed those to %# for the hex., and that
second umove is still &uc - it's still part of the full structure.

> 
> > +           }
> > +           break;
> 
> As (your edition of) this parser has already printed something at this
> point, it has to return 1 + RVAL_something to let the caller know,
> otherwise the address will be printed twice.  You can easily reproduce
> this by invoking UFFDIO_COPY with NULL address.
> 
> > +   }
> > +#endif
> > +
> > +   default:
> > +           return 0;
> 
> As this parser in default case is not going to do anything on exiting,
> it report this to the caller by returning RVAL_DECODED.

OK, I think.

and thanks for the pointer to that explanatory email.

I'll post an updated version.

Thanks,

Dave

> 
> 
> -- 
> ldv



> ------------------------------------------------------------------------------
> Find and fix application performance issues faster with Applications Manager
> Applications Manager provides deep performance insights into multiple tiers of
> your business applications. It resolves application problems quickly and
> reduces your MTTR. Get your free trial!
> https://ad.doubleclick.net/ddm/clk/302982198;130105516;z

> _______________________________________________
> Strace-devel mailing list
> Strace-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/strace-devel

--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel

Reply via email to