From mboxrd@z Thu Jan 1 00:00:00 1970 From: Helge Deller Subject: Re: [PATCH] enhance usability of /proc/sys/net/ipv4/ip_local_reserved_ports (v2) Date: Wed, 14 Mar 2012 23:06:33 +0100 Message-ID: <4F611669.6000903@gmx.de> References: <4F5BE563.9050506@gmx.de> <4F5FAF28.5030205@gmx.de> <1331711033.17983.19.camel@cr0> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Octavian Purdila , netdev@vger.kernel.org, David Miller , Andrew Morton , "Eric W. Biederman" , Frank Danapfel , Laszlo Ersek To: Cong Wang Return-path: Received: from mailout-de.gmx.net ([213.165.64.23]:41899 "HELO mailout-de.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1761517Ab2CNWGk (ORCPT ); Wed, 14 Mar 2012 18:06:40 -0400 In-Reply-To: <1331711033.17983.19.camel@cr0> Sender: netdev-owner@vger.kernel.org List-ID: On 03/14/2012 08:43 AM, Cong Wang wrote: > On Tue, 2012-03-13 at 21:33 +0100, Helge Deller wrote: >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index f487f25..1f60398 100644 >> --- a/kernel/sysctl.c >> +++ b/kernel/sysctl.c >> @@ -2805,6 +2805,8 @@ static int proc_do_cad_pid(struct ctl_table *table, int write, >> * 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. >> + * If "add" or "release" is written in front of numbers or number ranges, >> + * the given bits will be added to or released from the existing bitmap. >> * > > What if I only write "add" or "release" ("add ", "release " too) into > this file? Make sure you have tested this corner case. Sure, I tested this case. It will not modify the the current port list. But there were other cases which I initially didn't took care of, mostly because I wanted to keep the parser simple. Now, in the next version of the patch the following cases will be handled correctly: - "add release 100" (->syntax error) - "release 100 add 100" (->with all in one line the result will not be as expected because first bitmap_or() and then bitmap_andnot() will be executed, so that the 100th bit becomes released instead of added. Users will need to split this into two echo commands otherwise -EINVAL will be returned.) >> * Returns 0 on success. >> */ >> @@ -2813,11 +2815,13 @@ int proc_do_large_bitmap(struct ctl_table *table, int write, >> { >> int err = 0; >> bool first = 1; >> + bool add_or_release = 0, xrelease = 0; >> size_t left = *lenp; >> unsigned long bitmap_len = table->maxlen; >> unsigned long *bitmap = (unsigned long *) table->data; >> - unsigned long *tmp_bitmap = NULL; >> - char tr_a[] = { '-', ',', '\n' }, tr_b[] = { ',', '\n', 0 }, c; >> + unsigned long *tmp_bitmap = NULL, *release_bitmap = NULL; >> + char tr_a[] = { '-', ',', ' ', '\n' }, >> + tr_b[] = { ',', ' ', '\n', 0 }, c; >> >> if (!bitmap_len || !left || (*ppos && !write)) { >> *lenp = 0; >> @@ -2841,8 +2845,9 @@ int proc_do_large_bitmap(struct ctl_table *table, int write, >> } >> kbuf[left] = 0; >> >> - tmp_bitmap = kzalloc(BITS_TO_LONGS(bitmap_len) * sizeof(unsigned long), >> - GFP_KERNEL); >> + tmp_bitmap = kzalloc(2 * BITS_TO_LONGS(bitmap_len) * >> + sizeof(unsigned long), GFP_KERNEL); >> + release_bitmap = &tmp_bitmap[BITS_TO_LONGS(bitmap_len)]; > > So you double the size, and give the second half to 'release_bitmap', > this will waste spaces when release_bitmap is short, right? Yes. > *I think* we > can check if we want to release any bitmaps first, and then only > allocate one of tmp_bitmap and release_bitmap. The simpliest solution would be to use strcasestr(kbuf,"release") but this function isn't available in the kernel. Alternatively I could just search for e.g. upper- and lowercase "release", but I don't like that either. Maybe you have a better idea? Overall, 65536 bits (ports) for the ip_local_reserved_ports bitfield occupies 8K. With my patch this now becomes 16K. For desktop/server usages I think this is OK, esp. since it's only used temporarily and freed directly after usage again. >> if (!tmp_bitmap) { >> free_page(page); >> return -ENOMEM; >> @@ -2850,7 +2855,32 @@ int proc_do_large_bitmap(struct ctl_table *table, int write, >> proc_skip_char(&kbuf, &left, '\n'); >> while (!err && left) { >> unsigned long val_a, val_b; >> - bool neg; >> + bool neg, found; >> + >> + left -= proc_skip_spaces(&kbuf); >> + if (!left) >> + continue; >> + >> + if (first || add_or_release) { >> + found = (0 == strnicmp(kbuf, "add ", 4)); >> + if (found) { > > I think we don't need an extra variable 'found' here. Yes, thanks. Fixed in next version. Thanks, Helge