svn commit: r327699 - head/sys/sys

[Rodney W. Grimes]

[ Charset UTF-8 unsupported, converting... ]
> 
> On 08/01/2018 11:09, Rodney W. Grimes wrote:
> >> Author: pfg
> >> Date: Mon Jan  8 15:54:29 2018
> >> New Revision: 327699
> >> URL: https://svnweb.freebsd.org/changeset/base/327699
> >>
> >> Log:
> >>    Revert r327697:
> >>    malloc(9): drop the __result_use_check attribute for the kernel allocator.
> >>    
> >>    My bad: __result_use_check just checks the for the general and we always
> >>    want to make sure allocated memory is used, not only checked for nullness.
> >>    
> >>    Add it to reallocf since that was missing.
> > Please try not to combine a revert with an add, it makes it messy
> > to try and figure out things in the future when only the svn log
> > is being used to analyze stuff as digging in mail archives becomes
> > painful.
> >
> > Revert, then commit the add standalone, is the better sequence in
> > this type of situation.
> 
> Not that any of this is defined in our committers guide but IMHO "svn 
> revert" is just a tool, pretty much as "svn move" and "svn copy". The 
> idea is to make a committers' life easier: making two commits for such a 
> simple change is not "easier".

It is easier for other commiters to clearly see now, and in the future,
exactly what was what.  Please do not only think of yourself making
the commit, think of all the others that try to read the diffs and
understand exactly what is being done and why it is being done.

By seperating the revert from the change one reading these can quickly
go, yep that simple reverted what was done and move past it.  And then
when the commit for the new change comes in it is clear that yes, that
does exactly what it says it does.

Also your now forced to merge a bad commit, and a revert of that bad
commit+delta to do a MFC.  If this had been done with a pure revert
only and then a commit of the correct fix you could of just merged
the correct fix.

> 
> In this case the change is rather consistent: I added __result_use_check 
> to the three functions that needed it.

The change is mixed in with the noise of a revert.

> The commit log is not only clear on why the revert happened but also 
> explains the additional one line change.

I did not say the commit log was unclear, your correct, it is clear,
I am saying that the operation mechanism here is general just a
poor way to work in a vcs.

> When I MFC it, I will merge both changes for repository consistency but 
> the log will basically mention that I am adding __result_use_check to 
> reallocf().

And what well the mergeinfo be?

> 
> Pedro.
> >> Modified:
> >>    head/sys/sys/malloc.h
> >>
> >> Modified: head/sys/sys/malloc.h
> >> ==============================================================================
> >> --- head/sys/sys/malloc.h	Mon Jan  8 15:41:49 2018	(r327698)
> >> +++ head/sys/sys/malloc.h	Mon Jan  8 15:54:29 2018	(r327699)
> >> @@ -176,7 +176,7 @@ void	*contigmalloc(unsigned long size, struct malloc_t
> >>   	    __alloc_size(1) __alloc_align(6);
> >>   void	free(void *addr, struct malloc_type *type);
> >>   void	*malloc(unsigned long size, struct malloc_type *type, int flags)
> >> -	    __malloc_like __alloc_size(1);
> >> +	    __malloc_like __result_use_check __alloc_size(1);
> >>   void	*mallocarray(size_t nmemb, size_t size, struct malloc_type *type,
> >>   	    int flags) __malloc_like __result_use_check
> >>   	    __alloc_size(1) __alloc_size(2);
> >> @@ -187,9 +187,9 @@ void	malloc_type_freed(struct malloc_type *type, unsig
> >>   void	malloc_type_list(malloc_type_list_func_t *, void *);
> >>   void	malloc_uninit(void *);
> >>   void	*realloc(void *addr, unsigned long size, struct malloc_type *type,
> >> -	    int flags) __alloc_size(2);
> >> +	    int flags) __result_use_check __alloc_size(2);
> >>   void	*reallocf(void *addr, unsigned long size, struct malloc_type *type,
> >> -	    int flags) __alloc_size(2);
> >> +	    int flags) __result_use_check __alloc_size(2);
> >>   
> >>   struct malloc_type *malloc_desc2type(const char *desc);
> >>   #endif /* _KERNEL */
> >>
> >>
> 
> Pedro.
> 
> 

-- 
Rod Grimes                                                 rgrimes at freebsd.org