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

Serge Hallyn serge.hallyn at ubuntu.com
Thu Jan 7 19:20:24 UTC 2016


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):
> > > The initial check should use real lengths as with modulo a
> > > new required length of eg. 52 would be considered smaller
> > > than an old length of 48 (2 < 48).
> > > 
> > > To get the 'batches' count 'newlen' must be divided and not
> > > taken modulo BATCH_SIZE. Otherwise '101', which would need a
> > > 3rd batch to reach 150, would end up with two (2*50 = 100
> > > bytes) and thereby be truncated instead.
> > > 
> > > Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
> > > ---
> > >  cgfs.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/cgfs.c b/cgfs.c
> > > index 0659e9e..681a478 100644
> > > --- a/cgfs.c
> > > +++ b/cgfs.c
> > > @@ -75,9 +75,9 @@ static inline void drop_trailing_newlines(char *s)
> > >  static void dorealloc(char **mem, size_t oldlen, size_t newlen)
> > >  {
> > >  	int batches;
> > > -	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>
---
 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



More information about the lxc-devel mailing list