Changes to pfctl to allow easier integration into a library
Christian Mauderer
I'm currently working on a porting pfctl to a real-time operating
system. This is done using the same framework that Sebastian Huber
mentioned here (and probably at some other occasions):
https://lists.freebsd.org/pipermail/freebsd-hackers/2013-October/043553.html
We more or less want to use the pfctl program as a library. The
following changes simplify this. Obviously my problems are quiet similar
to the ones discussed in the linked topic.
- There has been a prototype that didn't quite match with its
implementation. A parameter was unsigned int instead of u_int32_t what
is the same on FreeBSD but not necessarily on a newlib based system.
- I added const qualifiers wherever they were applicable. This saves RAM
on a system where the code is executed from a ROM (like Flash).
- I had to pull some static variables out of their functions. This
allows to put them into an extra linker section without touching too
much of the code. We use the linker section to save / restore the
content before / after the program is called.
Would the attached patches be acceptable for integration into the
FreeBSD sources?
I've generated the patches against the git commit b6ff7c02cf9 on
https://github.com/freebsd/freebsd.git. Please tell me if another form
would be better.
Kind regards,
Christian Mauderer
--
--------------------------------------------
embedded brains GmbH
Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian...@embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax: +49-89-18 94 741 - 08
PGP: Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
Bjoern A. Zeeb
> Hello,
>
> I'm currently working on a porting pfctl to a real-time operating
> system. This is done using the same framework that Sebastian Huber
> mentioned here (and probably at some other occasions):
>
…
> Would the attached patches be acceptable for integration into the
> FreeBSD sources?
Hi,
(a) I’d prefer uintX_t to u_intX_t and I think FreeBSD is using the
former generally.
(b) All the variables you pulled out of functions that are not const,
would need to be virtualised for VIMAGE, as otherwise one virtual
network stack could affect another.
Would you be willing to look into this?
Gruesse
/bz
_______________________________________________
freebsd...@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hacke...@freebsd.org"
Christian Mauderer
> On 28 Jul 2016, at 14:03, Christian Mauderer wrote:
>
>> Hello,
>>
>> I'm currently working on a porting pfctl to a real-time operating
>> system. This is done using the same framework that Sebastian Huber
>> mentioned here (and probably at some other occasions):
>>
> …
>> Would the attached patches be acceptable for integration into the
>> FreeBSD sources?
>
> Hi,
>
> (a) I’d prefer uintX_t to u_intX_t and I think FreeBSD is using the
> former generally.
>
> (b) All the variables you pulled out of functions that are not const,
> would need to be virtualised for VIMAGE, as otherwise one virtual
> network stack could affect another.
>
> Would you be willing to look into this?
>
> Gruesse
> /bz
Hello Bjoern,
thanks for the feedback.
Regarding (a): Normally I would prefer the POSIX names from stdint.h
too. But it seems that the whole pfctl code uses the u_intX_t names.
Therefore I used this naming convention too. I think if the type names
are changed, this should be done in one commit for the whole pfctl code.
Should I create such a patch?
Regarding (b): Of course I can look into it. I only have the problem
that I didn't know VIMAGE before you mentioned it. I took a quick glance
at the following page:
https://wiki.freebsd.org/VIMAGE/porting-to-vimage
It seems to me that this is meant for kernel code only and not for a
user space application like pfctl. Did I miss something? Is the pfctl
code used directly in the kernel somewhere? Or is the virtualisation
also necessary for user space tools?
Please don't understand me wrong: I have no problem doing the work. On
contrary: As far as I can tell, the macro wrappers that are used in
files like sys/netinet/igmp.c could be useful for our porting effort
too. It seems that they wrap exactly the variables that are problematic
for us. So it would be at least simpler to identify the problematic
variables. It's even possible that we could use the macros to manipulate
the variables in the way we need.
Kind regards
Christian Mauderer
--
--------------------------------------------
embedded brains GmbH
Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian...@embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax: +49-89-18 94 741 - 08
PGP: Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
Christian Mauderer
> Hello,
>
[...]
> Would the attached patches be acceptable for integration into the
> FreeBSD sources?
>
> https://github.com/freebsd/freebsd.git. Please tell me if another form
> would be better.
>
> Kind regards,
>
> Christian Mauderer
>
I've got one additional patch: I made most of the global variables
static. They are used only inside the scope of one single c file.
Despite that, they have not been static. If I try to link the source
file into a library every non-static variables pollutes my name space.
Can I improve anything to make the patches more acceptable?
Is the virtualisation that Bjoern mentioned necessary or was my
interpretation correct that this is only meant for kernel space code?
Kind Regards
Kristof Provost
> Can I improve anything to make the patches more acceptable?
>
0003-pfctl-Pull-static-variables-out-of-the-function.patch is required?
I’m not sure I see why you need it.
> Is the virtualisation that Bjoern mentioned necessary or was my
> interpretation correct that this is only meant for kernel space code?
I believe your interpretations is correct.
User land code should not have to care about VIMAGE.
Regards,
Kristof
Christian Mauderer
> On 1 Aug 2016, at 15:03, Christian Mauderer wrote:
>> Can I improve anything to make the patches more acceptable?
>>
> Can you explain why
> 0003-pfctl-Pull-static-variables-out-of-the-function.patch is required?
> I’m not sure I see why you need it.
>
>> Is the virtualisation that Bjoern mentioned necessary or was my
>> interpretation correct that this is only meant for kernel space code?
> I believe your interpretations is correct.
> User land code should not have to care about VIMAGE.
>
> Regards,
> Kristof
Hello Kristof,
I use roughly the following method for the global variables:
- I put all initialized (zero or value) variables into a special named
linker section.
- In a wrapper around main() I do the following:
o First save the content of the section to a temporary memory space
o Execute the original (mostly unchanged) main()
o After main() finishes, I restore the content of the section
To simplify a later update to a newer source version, the difference
between the sources we use and the original FreeBSD sources should be
minimal. Therefore my attempt to put the variables into a section is the
following:
I create a header file (i.e. pfctl-data.h) that contains a matching
declaration of the global variables but with an added gcc attribute
__attribute__((__section__("my_section_name"))). This header file is
included at the end of the original pfctl.c.
Problem is: This method doesn't work for a static variable that is
defined inside a function. Therefore I pulled them out of the functions
and put them into the scope of the c module.
Kind regards
Christian Mauderer
--
--------------------------------------------
embedded brains GmbH
Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian...@embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax: +49-89-18 94 741 - 08
PGP: Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
Kristof Provost
On 1 Aug 2016, at 16:32, Christian Mauderer wrote:
> Am 01.08.2016 um 16:02 schrieb Kristof Provost:
>> On 1 Aug 2016, at 15:03, Christian Mauderer wrote:
>>> Can I improve anything to make the patches more acceptable?
>>>
>> Can you explain why
>> 0003-pfctl-Pull-static-variables-out-of-the-function.patch is
>> required?
>> I’m not sure I see why you need it.
>>
> I use roughly the following method for the global variables:
>
> - I put all initialized (zero or value) variables into a special named
> linker section.
> - In a wrapper around main() I do the following:
> o First save the content of the section to a temporary memory space
> o Execute the original (mostly unchanged) main()
> o After main() finishes, I restore the content of the section
>
> To simplify a later update to a newer source version, the difference
> between the sources we use and the original FreeBSD sources should be
> minimal. Therefore my attempt to put the variables into a section is
> the
> following:
>
> I create a header file (i.e. pfctl-data.h) that contains a matching
> declaration of the global variables but with an added gcc attribute
> __attribute__((__section__("my_section_name"))). This header file is
> included at the end of the original pfctl.c.
>
Oh.
Ick.
Clever, but … ick.
I’m not a big fan of this patch, because it makes the code a bit
harder to follow, rather than improving things as most of your other
patches do.
I suspect that something similar can be accomplished with a bit of
linker trickery.
A first idea is to insert two new symbols (e.g. pf_begin/pf_end) in two
separate files, the first before all of the pfctl object files, the
second after them.
This should let you group the .data section of the pfctl globals,
accomplishing what you do here with the __attribute__ attribute.
Regards,
Kristof
Christian Mauderer
> accomplishing what you do here with the *attribute* attribute.
>
> Regards,
> Kristof
>
Hello Kristof,
I agree that my solution is not optimal and that this specific patch
does not really improve the code for you. So I'll try to find alternatives.
The method you suggested would not work for us. We are using part of the
FreeBSD sources as a library that is statically linked with the rest of
the system. Using our build process, the order of the object files does
not guarantee an order of the symbols. As far as I know a fixed order
can only be achieved by the section names.
Theoretically it could be possible to get a similar result with some
object file manipulation (renaming sections, ...). I'll check if I'm
able to use such a solution instead and report back as soon as I can
tell you more.
Kind regards,
Christian Mauderer
--
--------------------------------------------
embedded brains GmbH
Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian...@embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax: +49-89-18 94 741 - 08
PGP: Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.