Re: [Xen-devel] [PATCH v4 3/7] add gettimeofday function to time managment

[Paul Semel <semelpaul@xxxxxxxxx>]

Hi !

Thanks a lot for reviewing !

On 04/13/2018 03:39 PM, Roger Pau Monné wrote:
> On Tue, Apr 10, 2018 at 09:16:57PM +0200, Paul Semel wrote:
> > this function acts as the POSIX gettimeofday function
> >
> > Signed-off-by: Paul Semel <phentex@xxxxxxxxx>
> > ---
> >
> > Notes:
> >      v4:
> >      - new patch version
> >
> >   common/time.c      | 30 ++++++++++++++++++++++++++++++
> >   include/xtf/time.h |  8 ++++++++
> >   2 files changed, 38 insertions(+)
> >
> > diff --git a/common/time.c b/common/time.c
> > index c1b7cd1..8489f3b 100644
> > --- a/common/time.c
> > +++ b/common/time.c
> > @@ -1,6 +1,7 @@
> >   #include <xtf/types.h>
> >   #include <xtf/traps.h>
> >   #include <xtf/time.h>
> > +#include <xen/errno.h>
> Sorting.
>
> >   
> >   #include <arch/barrier.h>
> >   #include <arch/lib.h>
> > @@ -109,6 +110,35 @@ uint64_t current_time(void)
> >       return sec + boot_time;
> >   }
> >   
> > +/* The POSIX gettimeofday syscall normally takes a second argument, which is
> > + * the timezone (struct timezone). However, it sould be NULL because linux
> > + * doesn't use it anymore. So we need for us to add it in this function
> > + */
> > +int gettimeofday(struct timeval *tp, void *restrict tzp)
> > +{
> > +    uint64_t boot_time, sec;
> > +    uint32_t mod, nsec;
> > +
> > +    if ( tzp != NULL )
> > +        return -EOPNOTSUPP;
> > +
> > +    if ( tp == NULL )
> > +        return -EINVAL;
> > +
> > +    get_time_info(&boot_time, &sec, &nsec);
> Why are you using get_time_info here? Shouldn't you use the
> current_time function introduced in the previous patch?
>
> Or else I don't see the need to introduce current_time in the previous
> patch.
Actually, I can't use *only* the current_time function here, because I won't be 
able to get the nanoseconds if so.
Anyway, in the last patch, I am using current_time function for the NOW() macro, 
which I think is really helpful.
Do you think I should drop all of those ?

> > +#if defined(__i386__)
> > +    mod = divmod64(&boot_time, SEC_TO_NSEC(1));
> > +#else
> > +    mod = boot_time % SEC_TO_NSEC(1);
> > +    boot_time /= SEC_TO_NSEC(1);
> > +#endif
> Please use divmod64 unconditionally.
>
> > +
> > +    tp->sec = sec + boot_time;
> > +    tp->nsec = nsec + mod;
> > +    return 0;
> > +}
> > +
> >   /*
> >    * Local variables:
> >    * mode: C
> > diff --git a/include/xtf/time.h b/include/xtf/time.h
> > index e33dc8a..ce4d6db 100644
> > --- a/include/xtf/time.h
> > +++ b/include/xtf/time.h
> > @@ -8,6 +8,12 @@
> >   
> >   #include <xtf/types.h>
> >   
> > +struct timeval {
> > +    uint64_t sec;
> > +    uint64_t nsec;
> > +};
> > +
> > +
> Extra newline.
Thanks,

--
Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel