[PATCH 1/5] logger: Change logger_offset() from macro to function

[PATCH 1/5] logger: Change logger_offset() from macro to function

[Tim Bird]

Convert to function and add log as a parameter, rather than relying
on log in the context of the macro.

Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
 drivers/staging/android/logger.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index ffc2d04..92456d7 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -60,7 +60,11 @@ struct logger_reader {
 };

 /* logger_offset - returns index 'n' into the log via (optimized) modulus */
-#define logger_offset(n)	((n) & (log->size - 1))
+size_t logger_offset(struct logger_log *log, size_t n)
+{
+	return n & (log->size-1);
+}
+

 /*
  * file_get_log - Given a file structure, return the associated log
@@ -137,7 +141,7 @@ static ssize_t do_read_log_to_user(struct logger_log *log,
 		if (copy_to_user(buf + len, log->buffer, count - len))
 			return -EFAULT;

-	reader->r_off = logger_offset(reader->r_off + count);
+	reader->r_off = logger_offset(log, reader->r_off + count);

 	return count;
 }
@@ -225,7 +229,7 @@ static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)

 	do {
 		size_t nr = get_entry_len(log, off);
-		off = logger_offset(off + nr);
+		off = logger_offset(log, off + nr);
 		count += nr;
 	} while (count < len);

@@ -260,7 +264,7 @@ static inline int clock_interval(size_t a, size_t b, size_t c)
 static void fix_up_readers(struct logger_log *log, size_t len)
 {
 	size_t old = log->w_off;
-	size_t new = logger_offset(old + len);
+	size_t new = logger_offset(log, old + len);
 	struct logger_reader *reader;

 	if (clock_interval(old, new, log->head))
@@ -286,7 +290,7 @@ static void do_write_log(struct logger_log *log, const void *buf, size_t count)
 	if (count != len)
 		memcpy(log->buffer, buf + len, count - len);

-	log->w_off = logger_offset(log->w_off + count);
+	log->w_off = logger_offset(log, log->w_off + count);

 }

@@ -311,7 +315,7 @@ static ssize_t do_write_log_from_user(struct logger_log *log,
 		if (copy_from_user(log->buffer, buf + len, count - len))
 			return -EFAULT;

-	log->w_off = logger_offset(log->w_off + count);
+	log->w_off = logger_offset(log, log->w_off + count);

 	return count;
 }
-- 
1.7.2.3

[PATCH 2/5] logger: simplify and optimize get_entry_len

[Tim Bird]

Make this code slightly easier to read, and eliminate calls
to sub-routines.  Some of these were previously optimized away
by the compiler, but one memcpy was not.

Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
 drivers/staging/android/logger.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 92456d7..92cfd94 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -93,19 +93,23 @@ static inline struct logger_log *file_get_log(struct file *file)
  * get_entry_len - Grabs the length of the payload of the next entry starting
  * from 'off'.
  *
+ * An entry length is 2 bytes (16 bits) in host endian order.
+ * In the log, the length does not include the size of the log entry structure.
+ * This function returns the size including the log entry structure.
+ *
  * Caller needs to hold log->mutex.
  */
 static __u32 get_entry_len(struct logger_log *log, size_t off)
 {
 	__u16 val;

-	switch (log->size - off) {
-	case 1:
-		memcpy(&val, log->buffer + off, 1);
-		memcpy(((char *) &val) + 1, log->buffer, 1);
-		break;
-	default:
-		memcpy(&val, log->buffer + off, 2);
+	if (unlikely(log->size - off == 1)) {
+		/* at end of buffer, handle wrap */
+		((unsigned char *)&val)[0] = log->buffer[off];
+		((unsigned char *)&val)[1] = log->buffer[0];
+	} else {
+		((unsigned char *)&val)[0] = log->buffer[off];
+		((unsigned char *)&val)[1] = log->buffer[off+1];
 	}

 	return sizeof(struct logger_entry) + val;
-- 
1.7.2.3

[PATCH 3/5] logger: reorder prepare_to_wait and mutex_lock

[Tim Bird]

If mutex_lock waits, it will return in state TASK_RUNNING,
rubbing out the effect of prepare_to_wait().

Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
 drivers/staging/android/logger.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 92cfd94..54b7cdf 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -172,9 +172,10 @@ static ssize_t logger_read(struct file *file, char __user *buf,

 start:
 	while (1) {
+		mutex_lock(&log->mutex);
+
 		prepare_to_wait(&log->wq, &wait, TASK_INTERRUPTIBLE);

-		mutex_lock(&log->mutex);
 		ret = (log->w_off == reader->r_off);
 		mutex_unlock(&log->mutex);
 		if (!ret)
-- 
1.7.2.3

[PATCH 4/5] logger: clarify code in clock_interval

[Tim Bird]

Add commentary, rename the function and make the code easier to read.

Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
 drivers/staging/android/logger.c |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 54b7cdf..8d9d4f1 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -242,16 +242,28 @@ static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)
 }

 /*
- * clock_interval - is a < c < b in mod-space? Put another way, does the line
- * from a to b cross c?
+ * is_between - is a < c < b, accounting for wrapping of a, b, and c
+ *    positions in the buffer
+ *
+ * That is, if a<b, check for c between a and b
+ * and if a>b, check for c outside (not between) a and b
+ *
+ * |------- a xxxxxxxx b --------|
+ *               c^
+ *
+ * |xxxxx b --------- a xxxxxxxxx|
+ *    c^
+ *  or                    c^
  */
-static inline int clock_interval(size_t a, size_t b, size_t c)
+static inline int is_between(size_t a, size_t b, size_t c)
 {
-	if (b < a) {
-		if (a < c || b >= c)
+	if (a < b) {
+		/* is c between a and b? */
+		if (a < c && c <= b)
 			return 1;
 	} else {
-		if (a < c && b >= c)
+		/* is c outside of b through a? */
+		if (c <= b || a < c)
 			return 1;
 	}

@@ -272,11 +284,11 @@ static void fix_up_readers(struct logger_log *log, size_t len)
 	size_t new = logger_offset(log, old + len);
 	struct logger_reader *reader;

-	if (clock_interval(old, new, log->head))
+	if (is_between(old, new, log->head))
 		log->head = get_next_entry(log, log->head, len);

 	list_for_each_entry(reader, &log->readers, list)
-		if (clock_interval(old, new, reader->r_off))
+		if (is_between(old, new, reader->r_off))
 			reader->r_off = get_next_entry(log, reader->r_off, len);
 }

-- 
1.7.2.3

[PATCH 5/5] logger: clarify non-update of w_off in do_write_log_from_user

[Tim Bird]

Add comment to explain when w_off is not updated in case of failed second
fragment copy to buffer.

Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
 drivers/staging/android/logger.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 8d9d4f1..115d8ed 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -330,6 +330,12 @@ static ssize_t do_write_log_from_user(struct logger_log *log,

 	if (count != len)
 		if (copy_from_user(log->buffer, buf + len, count - len))
+			/*
+			 * Note that by not updating w_off, this abandons the
+			 * portion of the new entry that *was* successfully
+			 * copied, just above.  This is intentional to avoid
+			 * message corruption from missing fragments.
+			 */
 			return -EFAULT;

 	log->w_off = logger_offset(log, log->w_off + count);
-- 
1.7.2.3

Re: [PATCH 1/5] logger: Change logger_offset() from macro to function

[Frank Rowand]

On 02/07/12 18:21, Tim Bird wrote:
> Convert to function and add log as a parameter, rather than relying
> on log in the context of the macro.
> 
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
> ---
>  drivers/staging/android/logger.c |   16 ++++++++++------
>  1 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index ffc2d04..92456d7 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -60,7 +60,11 @@ struct logger_reader {
>  };
> 
>  /* logger_offset - returns index 'n' into the log via (optimized) modulus */
> -#define logger_offset(n)	((n) & (log->size - 1))
> +size_t logger_offset(struct logger_log *log, size_t n)
> +{
> +	return n & (log->size-1);

        return n & (log->size - 1);
> +}
> +

Reviewed-by: Frank Rowand <frank.rowand@am.sony.com>

[PATCH 2/5 v2] logger: simplify and optimize get_entry_len

[Tim Bird]

Make this code slightly easier to read, and eliminate calls
to sub-routines.  Some of these were previously optimized away
by the compiler, but one memcpy was not.

In my testing, this makes the code about 20% smaller, and
has no sub-routine calls and no branches (on ARM).

v2 of this patch is, IMHO, easier to read than v1. Compared to
that patch it uses __u8 instead of unsigned char, for
consistency with the __u16 val data type, simplifies the
conditional expression, adds a another comment, and
moves a common statement out of the if.

Signed-off-by: Tim Bird <tim.bird@am.sony.com>
---
 drivers/staging/android/logger.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
index 92456d7..3475cb7 100644
--- a/drivers/staging/android/logger.c
+++ b/drivers/staging/android/logger.c
@@ -93,20 +93,24 @@ static inline struct logger_log *file_get_log(struct file *file)
  * get_entry_len - Grabs the length of the payload of the next entry starting
  * from 'off'.
  *
+ * An entry length is 2 bytes (16 bits) in host endian order.
+ * In the log, the length does not include the size of the log entry structure.
+ * This function returns the size including the log entry structure.
+ *
  * Caller needs to hold log->mutex.
  */
 static __u32 get_entry_len(struct logger_log *log, size_t off)
 {
 	__u16 val;

-	switch (log->size - off) {
-	case 1:
-		memcpy(&val, log->buffer + off, 1);
-		memcpy(((char *) &val) + 1, log->buffer, 1);
-		break;
-	default:
-		memcpy(&val, log->buffer + off, 2);
-	}
+	/* copy 2 bytes from buffer, in memcpy order, */
+	/* handling possible wrap at end of buffer */
+
+	((__u8 *)&val)[0] = log->buffer[off];
+	if (likely(off+1 < log->size))
+		((__u8 *)&val)[1] = log->buffer[off+1];
+	else
+		((__u8 *)&val)[1] = log->buffer[0];

 	return sizeof(struct logger_entry) + val;
 }
-- 
1.7.2.3

Re: [PATCH 1/5] logger: Change logger_offset() from macro to function

[Dima Zavin]

small style nit otherwise looks ok.

On Tue, Feb 7, 2012 at 6:21 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> Convert to function and add log as a parameter, rather than relying
> on log in the context of the macro.
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
> ---
>  drivers/staging/android/logger.c |   16 ++++++++++------
>  1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index ffc2d04..92456d7 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -60,7 +60,11 @@ struct logger_reader {
>  };
>
>  /* logger_offset - returns index 'n' into the log via (optimized) modulus */
> -#define logger_offset(n)       ((n) & (log->size - 1))
> +size_t logger_offset(struct logger_log *log, size_t n)
> +{
> +       return n & (log->size-1);

spaces around -

> +}
> +
>
>  /*
>  * file_get_log - Given a file structure, return the associated log
> @@ -137,7 +141,7 @@ static ssize_t do_read_log_to_user(struct logger_log *log,
>                if (copy_to_user(buf + len, log->buffer, count - len))
>                        return -EFAULT;
>
> -       reader->r_off = logger_offset(reader->r_off + count);
> +       reader->r_off = logger_offset(log, reader->r_off + count);
>
>        return count;
>  }
> @@ -225,7 +229,7 @@ static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)
>
>        do {
>                size_t nr = get_entry_len(log, off);
> -               off = logger_offset(off + nr);
> +               off = logger_offset(log, off + nr);
>                count += nr;
>        } while (count < len);
>
> @@ -260,7 +264,7 @@ static inline int clock_interval(size_t a, size_t b, size_t c)
>  static void fix_up_readers(struct logger_log *log, size_t len)
>  {
>        size_t old = log->w_off;
> -       size_t new = logger_offset(old + len);
> +       size_t new = logger_offset(log, old + len);
>        struct logger_reader *reader;
>
>        if (clock_interval(old, new, log->head))
> @@ -286,7 +290,7 @@ static void do_write_log(struct logger_log *log, const void *buf, size_t count)
>        if (count != len)
>                memcpy(log->buffer, buf + len, count - len);
>
> -       log->w_off = logger_offset(log->w_off + count);
> +       log->w_off = logger_offset(log, log->w_off + count);
>
>  }
>
> @@ -311,7 +315,7 @@ static ssize_t do_write_log_from_user(struct logger_log *log,
>                if (copy_from_user(log->buffer, buf + len, count - len))
>                        return -EFAULT;
>
> -       log->w_off = logger_offset(log->w_off + count);
> +       log->w_off = logger_offset(log, log->w_off + count);
>
>        return count;
>  }
> --
> 1.7.2.3
>

Re: [PATCH 2/5 v2] logger: simplify and optimize get_entry_len

[Dima Zavin]

On Wed, Feb 8, 2012 at 10:37 AM, Tim Bird <tim.bird@am.sony.com> wrote:
> Make this code slightly easier to read, and eliminate calls
> to sub-routines.  Some of these were previously optimized away
> by the compiler, but one memcpy was not.
>
> In my testing, this makes the code about 20% smaller, and
> has no sub-routine calls and no branches (on ARM).
>
> v2 of this patch is, IMHO, easier to read than v1. Compared to
> that patch it uses __u8 instead of unsigned char, for
> consistency with the __u16 val data type, simplifies the
> conditional expression, adds a another comment, and
> moves a common statement out of the if.
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
> ---
>  drivers/staging/android/logger.c |   20 ++++++++++++--------
>  1 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 92456d7..3475cb7 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -93,20 +93,24 @@ static inline struct logger_log *file_get_log(struct file *file)
>  * get_entry_len - Grabs the length of the payload of the next entry starting
>  * from 'off'.
>  *
> + * An entry length is 2 bytes (16 bits) in host endian order.
> + * In the log, the length does not include the size of the log entry structure.
> + * This function returns the size including the log entry structure.
> + *
>  * Caller needs to hold log->mutex.
>  */
>  static __u32 get_entry_len(struct logger_log *log, size_t off)
>  {
>        __u16 val;

Could using a union here make things look a little cleaner in the meat
of the function? Something like

union {
    __u16 s;
    __u8 b[2];
} val;

>
> -       switch (log->size - off) {
> -       case 1:
> -               memcpy(&val, log->buffer + off, 1);
> -               memcpy(((char *) &val) + 1, log->buffer, 1);
> -               break;
> -       default:
> -               memcpy(&val, log->buffer + off, 2);
> -       }
> +       /* copy 2 bytes from buffer, in memcpy order, */
> +       /* handling possible wrap at end of buffer */
> +
> +       ((__u8 *)&val)[0] = log->buffer[off];
> +       if (likely(off+1 < log->size))
> +               ((__u8 *)&val)[1] = log->buffer[off+1];

spaces around the + in 'off+1' in the above two lines.

> +       else
> +               ((__u8 *)&val)[1] = log->buffer[0];
>
>        return sizeof(struct logger_entry) + val;
>  }
> --
> 1.7.2.3
>

Re: [PATCH 3/5] logger: reorder prepare_to_wait and mutex_lock

[Dima Zavin]

On Tue, Feb 7, 2012 at 6:30 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> If mutex_lock waits, it will return in state TASK_RUNNING,
> rubbing out the effect of prepare_to_wait().
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>

Acked-by: Dima Zavin <dima@android.com>

> ---
>  drivers/staging/android/logger.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 92cfd94..54b7cdf 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -172,9 +172,10 @@ static ssize_t logger_read(struct file *file, char __user *buf,
>
>  start:
>        while (1) {
> +               mutex_lock(&log->mutex);
> +
>                prepare_to_wait(&log->wq, &wait, TASK_INTERRUPTIBLE);
>
> -               mutex_lock(&log->mutex);
>                ret = (log->w_off == reader->r_off);
>                mutex_unlock(&log->mutex);
>                if (!ret)
> --
> 1.7.2.3
>

Re: [PATCH 2/5 v2] logger: simplify and optimize get_entry_len

[Tim Bird]

On 2/8/2012 9:15 PM, Dima Zavin wrote:
> On Wed, Feb 8, 2012 at 10:37 AM, Tim Bird<tim.bird@am.sony.com>  wrote:
>> Make this code slightly easier to read, and eliminate calls
>> to sub-routines.  Some of these were previously optimized away
>> by the compiler, but one memcpy was not.
>>
>> In my testing, this makes the code about 20% smaller, and
>> has no sub-routine calls and no branches (on ARM).
>>
>> v2 of this patch is, IMHO, easier to read than v1. Compared to
>> that patch it uses __u8 instead of unsigned char, for
>> consistency with the __u16 val data type, simplifies the
>> conditional expression, adds a another comment, and
>> moves a common statement out of the if.
>>
>> Signed-off-by: Tim Bird<tim.bird@am.sony.com>
>> ---
>>   drivers/staging/android/logger.c |   20 ++++++++++++--------
>>   1 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
>> index 92456d7..3475cb7 100644
>> --- a/drivers/staging/android/logger.c
>> +++ b/drivers/staging/android/logger.c
>> @@ -93,20 +93,24 @@ static inline struct logger_log *file_get_log(struct file *file)
>>   * get_entry_len - Grabs the length of the payload of the next entry starting
>>   * from 'off'.
>>   *
>> + * An entry length is 2 bytes (16 bits) in host endian order.
>> + * In the log, the length does not include the size of the log entry structure.
>> + * This function returns the size including the log entry structure.
>> + *
>>   * Caller needs to hold log->mutex.
>>   */
>>   static __u32 get_entry_len(struct logger_log *log, size_t off)
>>   {
>>         __u16 val;
> Could using a union here make things look a little cleaner in the meat
> of the function? Something like
>
> union {
>      __u16 s;
>      __u8 b[2];
> } val;
>
That's a good idea.  I was looking for a way to use a byte array that
wouldn't get misaligned if the function declaration changed.  I'll
test this out and see what it looks like.
>> -       switch (log->size - off) {
>> -       case 1:
>> -               memcpy(&val, log->buffer + off, 1);
>> -               memcpy(((char *)&val) + 1, log->buffer, 1);
>> -               break;
>> -       default:
>> -               memcpy(&val, log->buffer + off, 2);
>> -       }
>> +       /* copy 2 bytes from buffer, in memcpy order, */
>> +       /* handling possible wrap at end of buffer */
>> +
>> +       ((__u8 *)&val)[0] = log->buffer[off];
>> +       if (likely(off+1<  log->size))
>> +               ((__u8 *)&val)[1] = log->buffer[off+1];
> spaces around the + in 'off+1' in the above two lines.
Yeah.  I keep omitting spaces.  I'll fix this, and the ones
mentioned on patch 1/5.
  -- Tim

Re: [PATCH 4/5] logger: clarify code in clock_interval

[Dima Zavin]

To be honest, i don't find the new logic code much different or
cleaner than the old code. I think just documenting (as well as the
function rename) as you've done without inverting the logic would be
sufficient. The ascii art helps immensely.

--Dima

On Tue, Feb 7, 2012 at 6:32 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> Add commentary, rename the function and make the code easier to read.
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>
> ---
>  drivers/staging/android/logger.c |   28 ++++++++++++++++++++--------
>  1 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 54b7cdf..8d9d4f1 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -242,16 +242,28 @@ static size_t get_next_entry(struct logger_log *log, size_t off, size_t len)
>  }
>
>  /*
> - * clock_interval - is a < c < b in mod-space? Put another way, does the line
> - * from a to b cross c?
> + * is_between - is a < c < b, accounting for wrapping of a, b, and c
> + *    positions in the buffer
> + *
> + * That is, if a<b, check for c between a and b
> + * and if a>b, check for c outside (not between) a and b
> + *
> + * |------- a xxxxxxxx b --------|
> + *               c^
> + *
> + * |xxxxx b --------- a xxxxxxxxx|
> + *    c^
> + *  or                    c^
>  */
> -static inline int clock_interval(size_t a, size_t b, size_t c)
> +static inline int is_between(size_t a, size_t b, size_t c)
>  {
> -       if (b < a) {
> -               if (a < c || b >= c)
> +       if (a < b) {
> +               /* is c between a and b? */
> +               if (a < c && c <= b)
>                        return 1;
>        } else {
> -               if (a < c && b >= c)
> +               /* is c outside of b through a? */
> +               if (c <= b || a < c)
>                        return 1;
>        }
>
> @@ -272,11 +284,11 @@ static void fix_up_readers(struct logger_log *log, size_t len)
>        size_t new = logger_offset(log, old + len);
>        struct logger_reader *reader;
>
> -       if (clock_interval(old, new, log->head))
> +       if (is_between(old, new, log->head))
>                log->head = get_next_entry(log, log->head, len);
>
>        list_for_each_entry(reader, &log->readers, list)
> -               if (clock_interval(old, new, reader->r_off))
> +               if (is_between(old, new, reader->r_off))
>                        reader->r_off = get_next_entry(log, reader->r_off, len);
>  }
>
> --
> 1.7.2.3
>

Re: [PATCH 5/5] logger: clarify non-update of w_off in do_write_log_from_user

[Dima Zavin]

On Tue, Feb 7, 2012 at 6:34 PM, Tim Bird <tim.bird@am.sony.com> wrote:
> Add comment to explain when w_off is not updated in case of failed second
> fragment copy to buffer.
>
> Signed-off-by: Tim Bird <tim.bird@am.sony.com>

Acked-by: Dima Zavin <dima@android.com>

> ---
>  drivers/staging/android/logger.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/staging/android/logger.c b/drivers/staging/android/logger.c
> index 8d9d4f1..115d8ed 100644
> --- a/drivers/staging/android/logger.c
> +++ b/drivers/staging/android/logger.c
> @@ -330,6 +330,12 @@ static ssize_t do_write_log_from_user(struct logger_log *log,
>
>        if (count != len)
>                if (copy_from_user(log->buffer, buf + len, count - len))
> +                       /*
> +                        * Note that by not updating w_off, this abandons the
> +                        * portion of the new entry that *was* successfully
> +                        * copied, just above.  This is intentional to avoid
> +                        * message corruption from missing fragments.
> +                        */
>                        return -EFAULT;
>
>        log->w_off = logger_offset(log, log->w_off + count);
> --
> 1.7.2.3
>