[lxc-devel] [PATCH lxcfs 4/5] cgfs: fix dorealloc's batch allocation

[Wolfgang Bumiller]

> On January 7, 2016 at 8:20 PM Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> Quoting Wolfgang Bumiller (w.bumiller at proxmox.com):
> > > On January 7, 2016 at 7:42 PM Serge Hallyn <serge.hallyn at ubuntu.com>
> > > wrote:
> > > Quoting Wolfgang Bumiller (w.bumiller at proxmox.com):
> > > > -	if (newlen % BATCH_SIZE <= oldlen % BATCH_SIZE)
> > > > +	if (newlen <= oldlen)
> > > 
> > > This will result in extra reallocs (though innocuous).
> > > Is there any reason not to just do
> > > 
> > > 	if (newlen / BATCH_SIZE <= oldlen / BATCH_SIZE)
> > 
> > Ah yes I missed that the old length is a string length not the
> > capacity, so yes, the division makes sense. Just not the modulus.
> > 
> 
> Ok, thanks - per your patch I think the following makes a nice cleanup:
>
> From a72193ff0dba319c3039372bc038708a75d0582f Mon Sep 17 00:00:00 2001
> From: Serge Hallyn <serge.hallyn at ubuntu.com>
> Date: Thu, 7 Jan 2016 11:17:17 -0800
> Subject: [PATCH 1/1] dorealloc: avoid extra reallocs
> 
> The original check was very wrong, using % instead of /.  However
> the length we track is the actual used length, not the allocated
> length, which is always (len / BATCH_SIZE) + 1.  We don't want
> to realloc when newlen is between oldlen and (oldlen / BATCH_SIZE) + 1)
> 
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>

Yes this seems fine and easier to read. Although the commit message
mixes length and batch count again ;-).

Do you care about a Reviewed-by line? If so

Reviewed-by: Wolfgang Bumiller <w.bumiller at proxmox.com>

> ---
>  cgfs.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/cgfs.c b/cgfs.c
> index ec0f630..f37bc2b 100644
> --- a/cgfs.c
> +++ b/cgfs.c
> @@ -74,18 +74,20 @@ static inline void drop_trailing_newlines(char *s)
>  #define BATCH_SIZE 50
>  static void dorealloc(char **mem, size_t oldlen, size_t newlen)
>  {
> -	int batches;
> -	if (newlen <= oldlen)
> +	int newbatches = (newlen / BATCH_SIZE) + 1;
> +	int oldbatches = (olden / BATCH_SIZE) + 1;
> +
> +	if (newbatches <= oldbatches)
>  		return;
> -	batches = (newlen / BATCH_SIZE) + 1;
> +
>  	if (!*mem) {
>  		do {
> -			*mem = malloc(batches * BATCH_SIZE);
> +			*mem = malloc(newbatches * BATCH_SIZE);
>  		} while (!*mem);
>  	} else {
>  		char *tmp;
>  		do {
> -			tmp = realloc(*mem, batches * BATCH_SIZE);
> +			tmp = realloc(*mem, newbatches * BATCH_SIZE);
>  		} while (!tmp);
>  		*mem = tmp;
>  	}
> -- 
> 2.5.0