Message ID | 201002221829.13987.opurdila@ixiacom.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
Octavian Purdila wrote: > > Here is a new version of this patch which fixes both the comma and invalid > value issues, please give it a try. > Sorry, it is even worse. :( > [net-next PATCH v5 2/3] sysctl: add proc_do_large_bitmap > > The new function can be used to read/write large bitmaps via /proc. A > comma separated range format is used for compact output and input > (e.g. 1,3-4,10-10). > Writing "50000-50100" gets EINVAL, it should be success. Writing "50000,50100" fails too. Please, at least, do some basic testing. Also some comments below. > Writing into the file will first reset the bitmap then update it > based on the given input. > > Signed-off-by: Octavian Purdila <opurdila@ixiacom.com> > Cc: WANG Cong <amwang@redhat.com> > Cc: Eric W. Biederman <ebiederm@xmission.com> Please resend the whole patchset, and update the doc in patch 3/3. > --- > include/linux/sysctl.h | 2 + > kernel/sysctl.c | 134 > ++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 136 insertions(+), 0 deletions(-) > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index f66014c..7bb5cb6 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -980,6 +980,8 @@ extern int proc_doulongvec_minmax(struct ctl_table *, int, > void __user *, size_t *, loff_t *); > extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int, > void __user *, size_t *, loff_t *); > +extern int proc_do_large_bitmap(struct ctl_table *, int, > + void __user *, size_t *, loff_t *); > > /* > * Register a set of sysctl names by calling register_sysctl_table > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 5259727..d8ea839 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -2635,6 +2635,140 @@ static int proc_do_cad_pid(struct ctl_table *table, > int write, The above line is wrong, it should be a part of previous line. > return 0; > } > > +/** > + * proc_do_large_bitmap - read/write from/to a large bitmap > + * @table: the sysctl table > + * @write: %TRUE if this is a write to the sysctl file > + * @buffer: the user buffer > + * @lenp: the size of the user buffer > + * @ppos: file position > + * > + * The bitmap is stored at table->data and the bitmap length (in bits) > + * in table->maxlen. > + * > + * We use a range comma separated format (e.g. 1,3-4,10-10) so that > + * large bitmaps may be represented in a compact manner. Writing into > + * the file will clear the bitmap then update it with the given input. > + * > + * Returns 0 on success. > + */ > +int proc_do_large_bitmap(struct ctl_table *table, int write, > + void __user *_buffer, size_t *lenp, loff_t *ppos) > +{ > + bool first = 1; > + unsigned long *bitmap = (unsigned long *) table->data; > + unsigned long bitmap_len = table->maxlen; > + int left = *lenp, err = 0; 'left' should be size_t. > + char __user *buffer = (char __user *) _buffer; > + char tr_a[] = { '-', ',', 0 }, tr_b[] = { ',', 0 }, c; > + > + > + if (!bitmap_len || !left || (*ppos && !write)) { > + *lenp = 0; > + return 0; > + } > + > + if (write) { > + err = proc_skip_wspace(&buffer, &left); > + while (!err && left) { > + unsigned long val_a, val_b; > + bool neg; > + > + err = proc_get_ulong(&buffer, &left, tr_a, > + &val_a, &neg, &c); > + if (err) > + break; > + > + if (val_a >= bitmap_len || neg) { > + err = -EINVAL; > + break; > + } > + > + val_b = val_a; > + if (left && c == '-') { > + /* skip the - */ > + buffer++; left--; > + > + err = proc_get_ulong(&buffer, &left, tr_b, > + &val_b, &neg, &c); > + if (err) > + break; > + > + if (val_b >= bitmap_len || neg || > + val_a > val_b) { > + err = -EINVAL; > + break; > + } > + } > + > + if (left) { > + err = proc_skip_wspace(&buffer, &left); > + if (err) > + break; > + if (left) { > + if (get_user(c, buffer)) { > + err = -EFAULT; > + break; > + } > + if (c != ',') { > + err = -EINVAL; > + break; > + } > + /* skip the , */ > + buffer++; left--; > + } > + } > + > + if (first) > + bitmap_clear(bitmap, 0, bitmap_len); > + > + while (val_a <= val_b) > + set_bit(val_a++, bitmap); > + > + first = 0; > + } > + if (!err) > + err = proc_skip_wspace(&buffer, &left); > + } else { > + unsigned long bit_a, bit_b = 0; > + > + while (left) { > + bit_a = find_next_bit(bitmap, bitmap_len, bit_b); > + if (bit_a >= bitmap_len) > + break; > + bit_b = find_next_zero_bit(bitmap, bitmap_len, > + bit_a + 1) - 1; > + > + err = proc_put_ulong(&buffer, &left, bit_a, 0, first, > + ','); > + if (err) > + break; > + if (bit_a != bit_b) { > + err = proc_put_char(&buffer, &left, '-'); > + if (err) > + break; > + err = proc_put_ulong(&buffer, &left, bit_b, 0, > + 1, 0); > + if (err) > + break; > + } > + > + first = 0; bit_b++; > + } > + if (!err) > + err = proc_put_char(&buffer, &left, '\n'); > + } > + > + if (first) { > + if (err) > + return err; > + bitmap_clear(bitmap, 0, bitmap_len); > + } > + *lenp -= left; > + *ppos += *lenp; > + return 0; > +} > + > #else /* CONFIG_PROC_FS */ > > int proc_dostring(struct ctl_table *table, int write, -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Cong Wang wrote: >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index 5259727..d8ea839 100644 >> --- a/kernel/sysctl.c >> +++ b/kernel/sysctl.c >> @@ -2635,6 +2635,140 @@ static int proc_do_cad_pid(struct ctl_table >> *table, int write, > > > The above line is wrong, it should be a part of previous line. > > Sorry, ignore this, it should be my thunderbird's fault, it truncated the line. I will fix it... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 24 February 2010 07:24:00 you wrote: > Octavian Purdila wrote: > > Here is a new version of this patch which fixes both the comma and > > invalid value issues, please give it a try. > > Sorry, it is even worse. :( > > > [net-next PATCH v5 2/3] sysctl: add proc_do_large_bitmap > > > > The new function can be used to read/write large bitmaps via /proc. A > > comma separated range format is used for compact output and input > > (e.g. 1,3-4,10-10). > > Writing "50000-50100" gets EINVAL, it should be success. > Writing "50000,50100" fails too. > Hmm, they don't fail for me :-/ > Please, at least, do some basic testing. > I do test them, I've attached the current test batch I was using. Anyways, today I've noticed that "1,2 3" does not fail and even more importantly the final value is "3". Being that I don't see a way of fixing this without not acknowledging 1,2 even though we will do set these values, I revisited the "1 2 3" issue. And I don't understand why this is actually an issue, we are just being more permissive (i.e. we are allowing as separators both whitespaces and ,). > Also some comments below. > > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > > index 5259727..d8ea839 100644 > > --- a/kernel/sysctl.c > > +++ b/kernel/sysctl.c > > @@ -2635,6 +2635,140 @@ static int proc_do_cad_pid(struct ctl_table > > *table, int write, > > The above line is wrong, it should be a part of previous line. > Probably my email client corrupted the patch, sorry about that, I will be more careful next time. > > + int left = *lenp, err = 0; > > 'left' should be size_t. > Will fix, thanks for catching this. I will resend the whole patch series once we get this formatting issue resolved.
Octavian Purdila wrote: > On Wednesday 24 February 2010 07:24:00 you wrote: >> Octavian Purdila wrote: >>> Here is a new version of this patch which fixes both the comma and >>> invalid value issues, please give it a try. >> Sorry, it is even worse. :( >> >>> [net-next PATCH v5 2/3] sysctl: add proc_do_large_bitmap >>> >>> The new function can be used to read/write large bitmaps via /proc. A >>> comma separated range format is used for compact output and input >>> (e.g. 1,3-4,10-10). >> Writing "50000-50100" gets EINVAL, it should be success. >> Writing "50000,50100" fails too. >> > > Hmm, they don't fail for me :-/ > >> Please, at least, do some basic testing. >> > > I do test them, I've attached the current test batch I was using. > > Anyways, today I've noticed that "1,2 3" does not fail and even more > importantly the final value is "3". > > Being that I don't see a way of fixing this without not acknowledging 1,2 even > though we will do set these values, I revisited the "1 2 3" issue. And I don't > understand why this is actually an issue, we are just being more permissive > (i.e. we are allowing as separators both whitespaces and ,). > > Oops, after rechecking my test case, it is actually my test case's fault. Sorry for this. I will fix my test case and run it again. > > I will resend the whole patch series once we get this formatting issue > resolved. > Thanks much! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Cong Wang wrote: > Octavian Purdila wrote: >> On Wednesday 24 February 2010 07:24:00 you wrote: >>> Octavian Purdila wrote: >>>> Here is a new version of this patch which fixes both the comma and >>>> invalid value issues, please give it a try. >>> Sorry, it is even worse. :( >>> >>>> [net-next PATCH v5 2/3] sysctl: add proc_do_large_bitmap >>>> >>>> The new function can be used to read/write large bitmaps via /proc. A >>>> comma separated range format is used for compact output and input >>>> (e.g. 1,3-4,10-10). >>> Writing "50000-50100" gets EINVAL, it should be success. >>> Writing "50000,50100" fails too. >>> >> >> Hmm, they don't fail for me :-/ >> >>> Please, at least, do some basic testing. >>> >> >> I do test them, I've attached the current test batch I was using. >> >> Anyways, today I've noticed that "1,2 3" does not fail and even more >> importantly the final value is "3". >> Being that I don't see a way of fixing this without not acknowledging >> 1,2 even though we will do set these values, I revisited the "1 2 3" >> issue. And I don't understand why this is actually an issue, we are >> just being more permissive (i.e. we are allowing as separators both >> whitespaces and ,). >> >> > > Oops, after rechecking my test case, it is actually my test case's > fault. Sorry for this. I will fix my test case and run it again. > Hi, Still a small problem, if I do write(fd, "50000,50100", 12) I will get a return value of 11, which should mean 11 bytes are written, however, actually only the first 6 bytes are accepted. The rest looks better now. Or am I missing something here? :) Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index f66014c..7bb5cb6 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -980,6 +980,8 @@ extern int proc_doulongvec_minmax(struct ctl_table *, int, void __user *, size_t *, loff_t *); extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int, void __user *, size_t *, loff_t *); +extern int proc_do_large_bitmap(struct ctl_table *, int, + void __user *, size_t *, loff_t *); /* * Register a set of sysctl names by calling register_sysctl_table diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 5259727..d8ea839 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2635,6 +2635,140 @@ static int proc_do_cad_pid(struct ctl_table *table, int write, return 0; } +/** + * proc_do_large_bitmap - read/write from/to a large bitmap + * @table: the sysctl table + * @write: %TRUE if this is a write to the sysctl file + * @buffer: the user buffer + * @lenp: the size of the user buffer + * @ppos: file position + * + * The bitmap is stored at table->data and the bitmap length (in bits) + * in table->maxlen. + * + * We use a range comma separated format (e.g. 1,3-4,10-10) so that + * large bitmaps may be represented in a compact manner. Writing into + * the file will clear the bitmap then update it with the given input. + * + * Returns 0 on success. + */ +int proc_do_large_bitmap(struct ctl_table *table, int write, + void __user *_buffer, size_t *lenp, loff_t *ppos) +{ + bool first = 1; + unsigned long *bitmap = (unsigned long *) table->data; + unsigned long bitmap_len = table->maxlen; + int left = *lenp, err = 0; + char __user *buffer = (char __user *) _buffer; + char tr_a[] = { '-', ',', 0 }, tr_b[] = { ',', 0 }, c; + + + if (!bitmap_len || !left || (*ppos && !write)) { + *lenp = 0; + return 0; + } + + if (write) { + err = proc_skip_wspace(&buffer, &left); + while (!err && left) { + unsigned long val_a, val_b; + bool neg; + + err = proc_get_ulong(&buffer, &left, tr_a, + &val_a, &neg, &c); + if (err) + break; + + if (val_a >= bitmap_len || neg) { + err = -EINVAL; + break; + } + + val_b = val_a; + if (left && c == '-') { + /* skip the - */ + buffer++; left--; + + err = proc_get_ulong(&buffer, &left, tr_b, + &val_b, &neg, &c); + if (err) + break; + + if (val_b >= bitmap_len || neg || + val_a > val_b) { + err = -EINVAL; + break; + } + } + + if (left) { + err = proc_skip_wspace(&buffer, &left); + if (err) + break; + if (left) { + if (get_user(c, buffer)) { + err = -EFAULT; + break; + } + if (c != ',') { + err = -EINVAL; + break; + } + /* skip the , */ + buffer++; left--; + } + } + + if (first) + bitmap_clear(bitmap, 0, bitmap_len); + + while (val_a <= val_b) + set_bit(val_a++, bitmap); + + first = 0; + } + if (!err) + err = proc_skip_wspace(&buffer, &left); + } else { + unsigned long bit_a, bit_b = 0; + + while (left) { + bit_a = find_next_bit(bitmap, bitmap_len, bit_b); + if (bit_a >= bitmap_len) + break; + bit_b = find_next_zero_bit(bitmap, bitmap_len, + bit_a + 1) - 1; + + err = proc_put_ulong(&buffer, &left, bit_a, 0, first, + ','); + if (err) + break; + if (bit_a != bit_b) { + err = proc_put_char(&buffer, &left, '-'); + if (err) + break; + err = proc_put_ulong(&buffer, &left, bit_b, 0, + 1, 0); + if (err) + break; + } + + first = 0; bit_b++; + } + if (!err) + err = proc_put_char(&buffer, &left, '\n'); + } + + if (first) { + if (err) + return err; + bitmap_clear(bitmap, 0, bitmap_len); + } + *lenp -= left; + *ppos += *lenp; + return 0; +} + #else /* CONFIG_PROC_FS */