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 <[email protected]>
---
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
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 <[email protected]>
---
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
If mutex_lock waits, it will return in state TASK_RUNNING,
rubbing out the effect of prepare_to_wait().
Signed-off-by: Tim Bird <[email protected]>
---
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
Add commentary, rename the function and make the code easier to read.
Signed-off-by: Tim Bird <[email protected]>
---
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
Add comment to explain when w_off is not updated in case of failed second
fragment copy to buffer.
Signed-off-by: Tim Bird <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
---
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
small style nit otherwise looks ok.
On Tue, Feb 7, 2012 at 6:21 PM, Tim Bird <[email protected]> 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 <[email protected]>
> ---
> ?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
>
On Wed, Feb 8, 2012 at 10:37 AM, Tim Bird <[email protected]> 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 <[email protected]>
> ---
> ?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
>
On Tue, Feb 7, 2012 at 6:30 PM, Tim Bird <[email protected]> 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 <[email protected]>
Acked-by: Dima Zavin <[email protected]>
> ---
> ?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
>
On 2/8/2012 9:15 PM, Dima Zavin wrote:
> On Wed, Feb 8, 2012 at 10:37 AM, Tim Bird<[email protected]> 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<[email protected]>
>> ---
>> 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
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 <[email protected]> wrote:
> Add commentary, rename the function and make the code easier to read.
>
> Signed-off-by: Tim Bird <[email protected]>
> ---
> ?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
>
On Tue, Feb 7, 2012 at 6:34 PM, Tim Bird <[email protected]> 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 <[email protected]>
Acked-by: Dima Zavin <[email protected]>
> ---
> ?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
>