> On January 8, 2016 at 11:23 AM Wolfgang Bumiller <[email protected]> > wrote: > > > > > On January 8, 2016 at 9:50 AM Wolfgang Bumiller <[email protected]> > > wrote: > > > > > > > > > On January 7, 2016 at 8:20 PM Serge Hallyn <[email protected]> > > > wrote: > > > Quoting Wolfgang Bumiller ([email protected]): > > > > > On January 7, 2016 at 7:42 PM Serge Hallyn <[email protected]> > > > > > wrote: > > > > > Quoting Wolfgang Bumiller ([email protected]): > > > > > > - 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 <[email protected]> > > > 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 <[email protected]> > > > > 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 <[email protected]> > > > > > --- > > > 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) > > Ah actually... if oldlen==0 (and newlen is < 50, as in most of the time > with the first line) then newbatches == oldbatches so the <= is true > and it won't do the first malloc(). > > realloc() also deals with NULL as first parameter so perhaps a > single conditional could do (unless that part of realloc() is not > standard C behavior or different on android or something?). > > Like so:
Sorry, it was supposed to be this one (forgot to add the file to the git commit --amend line before formatting the patch): From 24e98d74ca279ed2dc8e5a025add5a00737ba952 Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller <[email protected]> Date: Fri, 8 Jan 2016 11:09:57 +0100 Subject: [PATCH lxcfs] cgfs: make dorealloc allocate the first batch, too With a short first line the case can be *mem = NULL oldlen = 0 newlen = 5 (anything < 50) making newbatches == oldbatches == 1 causing the (newbatches <= oldbatches) condition to be true. Let realloc() handle *mem==NULL and use (!*mem || newbatches > oldbatches) as the only condition. Signed-off-by: Wolfgang Bumiller <[email protected]> --- cgfs.c | 9 +-------- lxcfs.c | 2 +- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/cgfs.c b/cgfs.c index 1f31ed1..e6330e3 100644 --- a/cgfs.c +++ b/cgfs.c @@ -77,14 +77,7 @@ static void dorealloc(char **mem, size_t oldlen, size_t newlen) int newbatches = (newlen / BATCH_SIZE) + 1; int oldbatches = (oldlen / BATCH_SIZE) + 1; - if (newbatches <= oldbatches) - return; - - if (!*mem) { - do { - *mem = malloc(newbatches * BATCH_SIZE); - } while (!*mem); - } else { + if (!*mem || newbatches > oldbatches) { char *tmp; do { tmp = realloc(*mem, newbatches * BATCH_SIZE); -- 2.1.4 _______________________________________________ lxc-devel mailing list [email protected] http://lists.linuxcontainers.org/listinfo/lxc-devel
