| Message ID | 20140112134024.2475a4f2@voldemort.scrye.com |
|---|---|
| State | RFC |
| Headers | show |
On 12 January 2014 21:40, Kevin Fenzi <kevin@scrye.com> wrote: > Greetings. > > I apologize in advance if this is not the right place to discuss this. > If it's not, please point me the right direction and I will move it > there. :) > > I am packaging up libnftables/nftables for Fedora. good to know! :) > > 2. There is some question about the /etc/nftables/* scripts. In Fedora > land, things in /etc/ should be config files, but these aren't really > config files. They call nft without a full path (/usr/sbin/nft, etc). > Should these really be in /usr/share ? or is it expected users will > modify them? Could you clarify the use case there? > There is a patch from me to address this: http://patchwork.ozlabs.org/patch/304866/ Feel free to test it and comment. I have the same issue in the Debian land. I applied the patch locally in the package as a workaround. The patch is not applied yet to upstream. > > 6. I recently enabled the xml stuff in libnftables and am seeing a > number of tests fail: > > parsing xmlfiles/55-rule-real.xml: [31mFAILED [0m (Invalid argument) > and > parsing xmlfiles/74-set.xml: [31mFAILED [0m (Invalid argument) > mxml: <!-- nft add rule filter output ct secmark 0 counter --> cannot > be a second root node after <nftables> > I can't see the libmxml version in the build log. Which version of libmxml are you using? Seem that your version of libmxml treats XML comments different than in my version (libmxml 2.6 from Debian). Also I guess you are using an outdated snapshot of libnftables. Some important changes happened to the XML/JSON parsers. Anyway, thanks for the report, I'm going to review the XML parser. > > Are these expected? The Invalid argument might be because it doesn't > have nftables available in the build kernel? But the json tests > work. :( > Not expected at all. These tests simply parse the XML/JSON input, they don't send anything to the kernel. > Thanks. Again, if I should send this somewhere else instead, just let > me know. Comments welcome here, direct email and/or in the above review > bugs. ;) I think this is the right place. Thanks.
[ re-sending as I forgot to cc the list ] On Sun, 12 Jan 2014 22:16:35 +0100 Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote: ...snip... > There is a patch from me to address this: > http://patchwork.ozlabs.org/patch/304866/ > Feel free to test it and comment. > > I have the same issue in the Debian land. I applied the patch locally > in the package as a workaround. > > The patch is not applied yet to upstream. Cool. That addresses the part of the issue where the interpreter isn't fully specified, but still the question is if these are config files that users are expected to modify or are noarch scripts provided by the package that are expected to be read-only (ie, /etc vs /usr/share)? > > 6. I recently enabled the xml stuff in libnftables and am seeing a > > number of tests fail: > > > > parsing xmlfiles/55-rule-real.xml: [31mFAILED [0m (Invalid > > argument) and > > parsing xmlfiles/74-set.xml: [31mFAILED [0m (Invalid argument) > > mxml: <!-- nft add rule filter output ct secmark 0 counter --> > > cannot be a second root node after <nftables> > > > > I can't see the libmxml version in the build log. Which version of > libmxml are you using? > Seem that your version of libmxml treats XML comments different than > in my version (libmxml 2.6 from Debian). 2.7 here. ;) So, likely that changed between 2.6 and 2.7? > Also I guess you are using an outdated snapshot of libnftables. Some > important changes happened to the XML/JSON parsers. Hum, thats the 2014-01-11 snapshot. Not very old. > Anyway, thanks for the report, I'm going to review the XML parser. Thank you. > > Are these expected? The Invalid argument might be because it doesn't > > have nftables available in the build kernel? But the json tests > > work. :( > > > > Not expected at all. > > These tests simply parse the XML/JSON input, they don't send anything > to the kernel. ok. Great > > Thanks. Again, if I should send this somewhere else instead, just > > let me know. Comments welcome here, direct email and/or in the > > above review bugs. ;) > > > I think this is the right place. Excellent. Thanks. kevin
[ re-sending as I forgot to cc the list ] On Sun, 12 Jan 2014 22:16:35 +0100 Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote: ...snip... > There is a patch from me to address this: > http://patchwork.ozlabs.org/patch/304866/ > Feel free to test it and comment. > > I have the same issue in the Debian land. I applied the patch locally > in the package as a workaround. > > The patch is not applied yet to upstream. Cool. That addresses the part of the issue where the interpreter isn't fully specified, but still the question is if these are config files that users are expected to modify or are noarch scripts provided by the package that are expected to be read-only (ie, /etc vs /usr/share)? > > 6. I recently enabled the xml stuff in libnftables and am seeing a > > number of tests fail: > > > > parsing xmlfiles/55-rule-real.xml: [31mFAILED [0m (Invalid > > argument) and > > parsing xmlfiles/74-set.xml: [31mFAILED [0m (Invalid argument) > > mxml: <!-- nft add rule filter output ct secmark 0 counter --> > > cannot be a second root node after <nftables> > > > > I can't see the libmxml version in the build log. Which version of > libmxml are you using? > Seem that your version of libmxml treats XML comments different than > in my version (libmxml 2.6 from Debian). 2.7 here. ;) So, likely that changed between 2.6 and 2.7? > Also I guess you are using an outdated snapshot of libnftables. Some > important changes happened to the XML/JSON parsers. Hum, thats the 2014-01-11 snapshot. Not very old. > Anyway, thanks for the report, I'm going to review the XML parser. Thank you. > > Are these expected? The Invalid argument might be because it doesn't > > have nftables available in the build kernel? But the json tests > > work. :( > > > > Not expected at all. > > These tests simply parse the XML/JSON input, they don't send anything > to the kernel. ok. Great > > Thanks. Again, if I should send this somewhere else instead, just > > let me know. Comments welcome here, direct email and/or in the > > above review bugs. ;) > > > I think this is the right place. Excellent. Thanks. kevin
On Sun, Jan 12, 2014 at 01:40:24PM -0700, Kevin Fenzi wrote: > 2. There is some question about the /etc/nftables/* scripts. In Fedora > land, things in /etc/ should be config files, but these aren't really > config files. They call nft without a full path (/usr/sbin/nft, etc). > Should these really be in /usr/share ? or is it expected users will > modify them? Could you clarify the use case there? These files are to be included, they are mainly executable for easier testing. > 4. nftables sets make to '-s' (ie, silent) on subdirs. It's good for > building packages to have verbose output in build logs. Would it be > possible to remove that? If not I can patch it out here. No problem adding an option to disable the silent output (I think current autoconf supports this), but I'd like to keep it as default. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 12, 2014 at 05:05:49PM -0700, Kevin Fenzi wrote: > [ re-sending as I forgot to cc the list ] > > On Sun, 12 Jan 2014 22:16:35 +0100 > Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote: > > ...snip... > > > There is a patch from me to address this: > > http://patchwork.ozlabs.org/patch/304866/ > > Feel free to test it and comment. > > > > I have the same issue in the Debian land. I applied the patch locally > > in the package as a workaround. > > > > The patch is not applied yet to upstream. > > Cool. > > That addresses the part of the issue where the interpreter isn't fully > specified, but still the question is if these are config files that > users are expected to modify or are noarch scripts provided by the > package that are expected to be read-only (ie, /etc vs /usr/share)? Yes, they are basically noarch scripts providing defaults for the table configurations. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 12, 2014 at 10:16:35PM +0100, Arturo Borrero Gonzalez wrote: > On 12 January 2014 21:40, Kevin Fenzi <kevin@scrye.com> wrote: > > > > 2. There is some question about the /etc/nftables/* scripts. In Fedora > > land, things in /etc/ should be config files, but these aren't really > > config files. They call nft without a full path (/usr/sbin/nft, etc). > > Should these really be in /usr/share ? or is it expected users will > > modify them? Could you clarify the use case there? > > > > There is a patch from me to address this: > http://patchwork.ozlabs.org/patch/304866/ > Feel free to test it and comment. > > I have the same issue in the Debian land. I applied the patch locally > in the package as a workaround. > > The patch is not applied yet to upstream. Could you please resubmit? I'm going to apply it to the master branch. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 12, 2014 at 01:40:24PM -0700, Kevin Fenzi wrote: > 3. nftables hard codes installing as root, which is no good for > building packages (patch attached that just removes the owner/group > setting there). Please resubmit with a subject, a brief changelog and a Signed-off-by: line and I'm going to apply it. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday 2014-01-12 21:40, Kevin Fenzi wrote: > >1. Completely minor, but noted in review of both packages that the >COPYING file has the old fsf address in it. Would be great if you could >update to the new one. >https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address No, best remove the address, because you really don't want to be updating all the time. >3. nftables hard codes installing as root, which is no good for >building packages (patch attached that just removes the owner/group >setting there). Well I sent an automake-making patch that would fix all those problems, but so far it has again been ignored to be merged. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 13, 2014 at 09:54:22AM +0100, Jan Engelhardt wrote: > > On Sunday 2014-01-12 21:40, Kevin Fenzi wrote: > > > >1. Completely minor, but noted in review of both packages that the > >COPYING file has the old fsf address in it. Would be great if you could > >update to the new one. > >https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address > > No, best remove the address, because you really don't want to be updating > all the time. "All the time" seems slightly exagerated. But the "updated" license linked in the wiki is exactly the same version we're using, the address hasn't been changed: http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 13 Jan 2014 09:09:43 +0000 Patrick McHardy <kaber@trash.net> wrote: > On Mon, Jan 13, 2014 at 09:54:22AM +0100, Jan Engelhardt wrote: > > > > On Sunday 2014-01-12 21:40, Kevin Fenzi wrote: > > > > > >1. Completely minor, but noted in review of both packages that the > > >COPYING file has the old fsf address in it. Would be great if you > > >could update to the new one. > > >https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address > > > > No, best remove the address, because you really don't want to be > > updating all the time. > > "All the time" seems slightly exagerated. But the "updated" license > linked in the wiki is exactly the same version we're using, the > address hasn't been changed: > > http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt It has: - Copyright (C) 1989, 1991 Free Software Foundation, Inc. - 675 Mass Ave, Cambridge, MA 02139, USA + Copyright (C) 1989, 1991 Free Software Foundation, Inc., + 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA ... and a few other diffs like saying <yeah> instead of 19xx and Lesser GNU Public License instead of Library, etc. FSF moved buildings a few years ago... as far as I know they don't plan to move again, and it's only happened once. The GPLv3 COPYING file I note doesn't have any physical address in it. ;) kevin
On Mon, Jan 13, 2014 at 12:18:36PM -0700, Kevin Fenzi wrote: > On Mon, 13 Jan 2014 09:09:43 +0000 > Patrick McHardy <kaber@trash.net> wrote: > > > On Mon, Jan 13, 2014 at 09:54:22AM +0100, Jan Engelhardt wrote: > > > > > > On Sunday 2014-01-12 21:40, Kevin Fenzi wrote: > > > > > > > >1. Completely minor, but noted in review of both packages that the > > > >COPYING file has the old fsf address in it. Would be great if you > > > >could update to the new one. > > > >https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address > > > > > > No, best remove the address, because you really don't want to be > > > updating all the time. > > > > "All the time" seems slightly exagerated. But the "updated" license > > linked in the wiki is exactly the same version we're using, the > > address hasn't been changed: > > > > http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt > > It has: > > - Copyright (C) 1989, 1991 Free Software Foundation, Inc. > - 675 Mass Ave, Cambridge, MA 02139, USA > + Copyright (C) 1989, 1991 Free Software Foundation, Inc., > + 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > > ... and a few other diffs like saying <yeah> instead of 19xx and Lesser > GNU Public License instead of Library, etc. That's what we have (and always had) in nftables: Copyright (C) 1989, 1991 Free Software Foundation, Inc. 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA > FSF moved buildings a few years ago... as far as I know they don't plan > to move again, and it's only happened once. The GPLv3 COPYING file I > note doesn't have any physical address in it. ;) > > kevin -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 14 Jan 2014 08:55:33 +0000
Patrick McHardy <kaber@trash.net> wrote:
> That's what we have (and always had) in nftables:
Indeed, it's libnftables that has the old one.
Sorry for the confusion...
kevin
Hi, On Sun, Jan 12, 2014 at 05:11:16PM -0700, Kevin Fenzi wrote: > [ re-sending as I forgot to cc the list ] > > On Sun, 12 Jan 2014 22:16:35 +0100 > Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> wrote: > > ...snip... > > > There is a patch from me to address this: > > http://patchwork.ozlabs.org/patch/304866/ > > Feel free to test it and comment. > > > > I have the same issue in the Debian land. I applied the patch locally > > in the package as a workaround. > > > > The patch is not applied yet to upstream. > > Cool. > > That addresses the part of the issue where the interpreter isn't fully > specified, but still the question is if these are config files that > users are expected to modify or are noarch scripts provided by the > package that are expected to be read-only (ie, /etc vs /usr/share)? > > > > 6. I recently enabled the xml stuff in libnftables and am seeing a > > > number of tests fail: > > > > > > parsing xmlfiles/55-rule-real.xml: [31mFAILED [0m (Invalid > > > argument) and > > > parsing xmlfiles/74-set.xml: [31mFAILED [0m (Invalid argument) > > > mxml: <!-- nft add rule filter output ct secmark 0 counter --> > > > cannot be a second root node after <nftables> > > > > > > > I can't see the libmxml version in the build log. Which version of > > libmxml are you using? > > Seem that your version of libmxml treats XML comments different than > > in my version (libmxml 2.6 from Debian). > > 2.7 here. ;) So, likely that changed between 2.6 and 2.7? @Arturo: please investigate this issue as the first release is coming soon, otherwise we'll have to restrict xml support to one of the libmxml versions. I think it would be better if libnftables supports latest version of libmxml as it's the natural move on most distributions to pack the last software version. Moreover, it would be good to contact the author to know if he has plans to keep a stable API/ABI in upcoming releases of libmxml, this is very important for third party software. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15 January 2014 10:53, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > @Arturo: please investigate this issue as the first release is coming > soon, otherwise we'll have to restrict xml support to one of the > libmxml versions. I think it would be better if libnftables supports > latest version of libmxml as it's the natural move on most > distributions to pack the last software version. Moreover, it would be > good to contact the author to know if he has plans to keep a stable > API/ABI in upcoming releases of libmxml, this is very important for > third party software. Sure, I plan to make the required changes in libnftables to support the latest version of libmxml.
diff -Nur nftables-0.0.orig/doc/Makefile.in nftables-0.0/doc/Makefile.in --- nftables-0.0.orig/doc/Makefile.in 2013-11-29 03:29:41.000000000 -0700 +++ nftables-0.0/doc/Makefile.in 2013-11-30 14:31:33.159267834 -0700 @@ -10,11 +10,11 @@ @echo -e " INSTALL\tdoc" if test -n "$(mandocs-y)"; then \ $(MKDIR_P) $(DESTDIR)/${mandir}/man8 ;\ - $(INSTALL) -m 755 -o root -g root $(mandocs-y) \ + $(INSTALL) -m 755 -p $(mandocs-y) \ $(DESTDIR)/${mandir}/man8/ ;\ fi if test -n "$(pdfdocs-y)"; then \ $(MKDIR_P) $(DESTDIR)/${pdfdir} ;\ - $(INSTALL) -m 755 -o root -g root $(pdfdocs-y) \ + $(INSTALL) -m 755 -p $(pdfdocs-y) \ $(DESTDIR)/${pdfdir}/ ;\ fi diff -Nur nftables-0.0.orig/files/Makefile.in nftables-0.0/files/Makefile.in --- nftables-0.0.orig/files/Makefile.in 2013-11-29 03:29:41.000000000 -0700 +++ nftables-0.0/files/Makefile.in 2013-11-30 14:30:35.440421941 -0700 @@ -1,4 +1,4 @@ install: @echo -e " INSTALL\tfiles" $(MKDIR_P) $(DESTDIR)/$(confdir) - $(INSTALL) -m 755 -o root -g root $(SUBDIR)nftables/* $(DESTDIR)/$(confdir)/ + $(INSTALL) -m 755 -p $(SUBDIR)nftables/* $(DESTDIR)/$(confdir)/ diff -Nur nftables-0.0.orig/Makefile.rules.in nftables-0.0/Makefile.rules.in --- nftables-0.0.orig/Makefile.rules.in 2013-11-29 03:29:41.000000000 -0700 +++ nftables-0.0/Makefile.rules.in 2013-11-30 14:26:03.461158244 -0700 @@ -61,7 +61,7 @@ $(1)-install: @echo -e " INSTALL\t$1" $(MKDIR_P) $$(DESTDIR)/$$($(1)-destdir) - $(INSTALL) -m 755 -o root -g root \ + $(INSTALL) -m 755 -p \ $(SUBDIR)$(1) \ $$(DESTDIR)/$$($(1)-destdir)/$(1) install_targets += $(1)-install