On 18 May 2015 at 18:14, Arnd Bergmann <[email protected]> wrote: > On Thursday 14 May 2015 15:03:10 Baolin Wang wrote: > > This patch changes the "settime" pointer's argument with timespec64 type > in security_operations > > structure, and also introduces the security_settime64() function with > timespec64 type, meanwhile > > moves the security_settime() defined in security/security.c file to > include/linux/security.h file > > as a static function. > > > > Also introduces a default security_settime64() function with timespec64 > type when it doesn't define > > the CONFIG_SECURITY macro. And change the cap_settime() function's > argument with timespec64 type, > > which is only used by security_settime64() function. > > Regarding the changelog: please start each patch changelog with a > description of the current behavior and what is wrong with it, only > then explain what you are doing to address it. You description above > only says what you do, not why, and that is not too helpful for someone > trying to dig through the git history later, or trying to review the > patch out of context, for that matter. > > Also, watch out for overly long lines. Try to make the subject no more > than 60 characters (as explained before), and limit the text lines to > about 70 characters. > > The patch itself looks good to me, just one small improvement I'd do: > > > @@ -1790,7 +1791,13 @@ int security_capable_noaudit(const struct cred > *cred, struct user_namespace *ns, > > int security_quotactl(int cmds, int type, int id, struct super_block > *sb); > > int security_quota_on(struct dentry *dentry); > > int security_syslog(int type); > > -int security_settime(const struct timespec *ts, const struct timezone > *tz); > > +int security_settime64(const struct timespec64 *ts, const struct > timezone *tz); > > +static int security_settime(const struct timespec *ts, const struct > timezone *tz) > > +{ > > + struct timespec64 ts64 = timespec_to_timespec64(*ts); > > + > > + return security_settime64(&ts64, tz); > > +} > > int security_vm_enough_memory_mm(struct mm_struct *mm, long pages); > > int security_bprm_set_creds(struct linux_binprm *bprm); > > int security_bprm_check(struct linux_binprm *bprm); > > @@ -2040,10 +2047,18 @@ static inline int security_syslog(int type) > > return 0; > > } > > > > +static inline int security_settime64(const struct timespec64 *ts, > > + const struct timezone *tz) > > +{ > > + return cap_settime(ts, tz); > > +} > > + > > static inline int security_settime(const struct timespec *ts, > > const struct timezone *tz) > > { > > - return cap_settime(ts, tz); > > + struct timsepc64 ts64 = timespec_to_timespec64(*ts); > > + > > + return security_settime64(&ts64, tz); > > } > > > > static inline int security_vm_enough_memory_mm(struct mm_struct *mm, > long pages) > > > You have added two identical definitions for security_settime(). I would > try to move that definition outside the #ifdef so you only need one of the > two. If that ends up being ugly, I'd simplify the second definition to > just call cap_settime() directly: security_settime64() is only a trivial > wrapper here, so you can do the cap_settime() call in security_settime() > and avoid the extra indirection (though the compiler won't care). > > Arnd >
Thanks for your comments. And the simplification for the second definition of security_settime() is very reasonable, I'll simplify it. As well as modify the changelog to more helpful. -- Baolin.wang Best Regards _______________________________________________ Y2038 mailing list [email protected] https://lists.linaro.org/mailman/listinfo/y2038
