[Home]

Summary:DAHLIN-00139: [patch] FreeBSD port
Reporter:Max Khon (max khon)Labels:
Date Opened:2009-09-10 16:53:20Date Closed:2009-11-11 11:13:51.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:dahdi (the module)
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) dahdi-kernel-nospace.diff
( 1) dahdi-tools.diff
( 2) loadzone-kernel.diff
( 3) loadzone-tools.diff
Description:zaptel-bsd was maintained in a separate SVN repository and syncing with main zaptel tree always was a pain.

I would like to submit dahdi FreeBSD port back to Digium for inclusion into main SVN tree, if possible.
Comments:By: Yuri (ys) 2009-09-11 03:36:28

>Max Khon

Please sign a license agreement, but without this, we can't see the patch files.
By: Max Khon (max khon) 2009-09-11 03:38:23

I signed the license yesterday. It is still in the status "PENDING".
By: Tzafrir Cohen (tzafrir) 2009-09-14 13:29:47

No issue with the tools fixes. But as for the "linux" fixes: will a single tree for both work?
By: Max Khon (max khon) 2009-09-14 22:16:26

Yes it will.

Echo cancellation and hw drivers differ only in init/probe/attach routines.

Main DAHDI driver is more complex, but still 99% of code there is not Linux-specific.

Can we have at least dahdi-tools patch to be committed now?
By: Digium Subversion (svnbot) 2009-09-15 12:52:53

Repository: dahdi
Revision: 7134

U   tools/trunk/Makefile
U   tools/trunk/dahdi_monitor.c
U   tools/trunk/tonezone.c

------------------------------------------------------------------------
r7134 | tzafrir | 2009-09-15 12:52:53 -0500 (Tue, 15 Sep 2009) | 7 lines

Fix some FreeBSD compatibility issues in -tools

Part of issue DAHLIN-139.
Reported by: Max Khon
Patches:
     dahdi-tools.diff uploaded by Max Khon (license 884)

------------------------------------------------------------------------

http://svn.digium.com/view/dahdi?view=rev&revision=7134
By: Tzafrir Cohen (tzafrir) 2009-09-15 12:54:17

I commited the small tools patch. Though I still see some remaining 'install -D'-s in Makefile .

The concensus seems to be that a separate dahdi-freebsd would be preffered.
By: Sean Bright (seanbright) 2009-09-15 15:44:38

<pre>
+#if defined(__FreeBSD__)
+    if ((res = ioctl(fd, DAHDI_LOADZONE, &h))) {
+#else
    if ((res = ioctl(fd, DAHDI_LOADZONE, h))) {
+#endif
</pre>
Why was this change necessary in tonezone.c?


By: Max Khon (max khon) 2009-09-16 01:37:01

seanbright: in FreeBSD ioctl arguments are copied to kernel space by kernel. So, parameters with variable length has to be passed this way (by passing their address).

There is one more bit missing there: DAHDI_LOADZONE ioctl should be changed to the following:

#define DAHDI_LOADZONE                  _IOW(DAHDI_CODE, 25, struct dahdi_tone_def_header *)

(pointer to header instead of header by value), but FreeBSD port historically used Linux declaration (this means that a few extra bytes are copied to kernel when this ioctl is called and there is a probablity of getting SIGSEGV).
By: Max Khon (max khon) 2009-09-16 01:48:57

tzafrir: can you let me know why the consensus is to have separate dahdi-freebsd branch?

1) If you are worried about a number of #ifdefs:

- the number of #ifdefs in echo cancellation and hw drivers can be minimized to just one #ifdef (which headers to include). init/probe/attach code can be moved to the appropriate *-linux.c and *-freebsd.c parts. This is purely mechanical job, because currently there are two #ifdefs in these drivers: one for headers and one for init/probe/attach.

- the number of #ifdefs in main DAHDI driver can be minimized further. I can add more compat schemes (OS abstraction) so that the number of #ifdefs will be minimized to one or two.

2) If you are worried that FreeBSD driver can slow down Linux driver development:

We can always consider FreeBSD driver as a Tier-2 driver. This means that it can be broken from time to time when active development is in progress. Moreover, FreeBSD users will most likely not use the driver directly from Digium distribution, but via FreeBSD port, which can be a staging area for FreeBSD patches (patches will be added to FreeBSD port first, then pushed down to Digium SVN)

3) If you are worried that FreeBSD driver will require resources to maintain:

I am willing to contribute and maintain FreeBSD port. There are also other developers out there (those that were active in FreeBSD zaptel-bsd and dahdi-bsd projects).

My opinion still is that single source tree is better because 99% of code is not OS-specific. There is a successful example: NVIDIA maintains FreeBSD/Linux drivers in a single source tree (with OS abstraction layer).
By: Max Khon (max khon) 2009-09-16 03:46:50

seanbright: Kevin wants to remove this diff between Linux and FreeBSD. I submitted a patch: http://lists.digium.com/pipermail/asterisk-dev/2009-September/039822.html
By: Sean Bright (seanbright) 2009-09-16 10:08:15

Max,

Please upload all patches through this interface (Mantis) so they can be reviewed and applied.

Thanks!
By: Max Khon (max khon) 2009-09-16 10:18:42

Sorry, forgot about license agreement. Done:

loadzone-kernel.diff: dahdi-kernel patch for DAHDI_LOADZONE
loadzone-tools.diff: dahdi-tools patch for DAHDI_LOADZONE
By: Shaun Ruffell (sruffell) 2009-09-16 11:00:38

Max, with regards to why a separate branch for FreeBSD is preferred (at least for myself, I can not speak for others) is that the dahdi-linux tree is moving in a direction as close to a style that could be included in mainline as possible.  Adding definitions for FreeBSD will confuse that effort.

However, I know it was discussed in passing that since the changes are relatively minor, setting up a separate dahdi-freebsd tree that could be automerged from the trunk may ease the maintenance burden.
By: Yuri (ys) 2009-09-16 11:14:05

Sruffell, before make separate branch for BSD, It is necessary to put in order the user space utils (tools) which do not concern the specific OS.
By: Shaun Ruffell (sruffell) 2009-09-16 11:34:33

ys: tzafrir has already started committing those patches to dahdi-tools so no problem there.
By: Max Khon (max khon) 2009-09-16 12:09:49

sruffell: thank you for the explanation. But please note that FreeBSD patch would be minimal only after my patch (or similar, that adds OS abstraction) is committed.

Also, as I already mentioned above: OS-specific parts should be placed to appropriate *-linux.[ch] or *-freebsd.[ch] files. And you will not need to push FreeBSD bits to mainline after that.
By: vadim (vadim) 2009-09-17 10:40:15

--- drivers/dahdi/dahdi-base.c (revision 7146)
+++ drivers/dahdi/dahdi-base.c (working copy)
@@ -4270,6 +4270,10 @@
return -EFAULT;
return dahdi_set_default_zone(j);
case DAHDI_LOADZONE:
+ if (copy_from_user(&data, (void *) data, sizeof(data)))
+ return -EFAULT;
+ /* FALLTHRU */
+ case DAHDI_LOADZONE_V1:
return ioctl_load_zone(data);
case DAHDI_FREEZONE:
get_user(j, (int *) data);

This call to copy_form_user appears to contain ENORMOUS bug....
'data' contains user-mode address  the call should be something like:

if (copy_from_user(&kernel_data_copy, (void *) data, sizeof(kernel_data_copy)))
By: Max Khon (max khon) 2009-09-17 10:52:53

vadim: yes, data contains user-space address, but data itself is allocated on stack.
By: vadim (vadim) 2009-09-17 11:38:13

Ok, i see.

This is kind of hackish... It will work  if data is of type int...  but if
later for some reason data will become some kind of structure this will not work.
I would avoid this type of code as it is REALLY non obvious
By: Max Khon (max khon) 2009-09-17 12:09:33

If we speak about Linux, DAHDI_LOADZONE ioctl argument is a pointer and "long" is guaranteed to be sufficient for storing a pointer (otherwise ioctl argument would not be "long").

The same applies to FreeBSD, where "data" is of "caddr_t" type.
By: vadim (vadim) 2009-09-17 12:32:00

But pointer to WHAT?  

Suppose the data contains value 0x8000FFF0
It means the somewhere in user space at address 0x8000FFF0 resides some data which need to be copied into the kernel address space.

Your call to copy_from_user will copy only 4 bytes  from user space to kernel space.   Is this what you want?  If so it is ok for now.

If later DAHDI_LOADZONE will require more than 4 bytes of data your code will break.
By: Max Khon (max khon) 2009-09-17 13:05:28

Yes, this is what I want.
By: Shaun Ruffell (sruffell) 2009-09-21 09:56:44

Max:  Even if the patch is minimal, I would still be more keen on maintaining it in a separate tree that is easily mergeable for the reasons articulated here: http://lwn.net/Articles/127669/.  Perhaps the Linux API could be used as an abstraction layer for BSD even (where it can)?
By: Max Khon (max khon) 2009-09-21 21:53:35

sruffell: I tried to use Linux API as much as possible already: e.g. I used Linux lists, Linux API for spinlocks and rwlocks and so on.

I think that it is possible to use Linux API for fops. I will try to make a new patch this week.

In the meantime:
- can someone review and commit loadzone diffs? It removes incompatibility between Linux and FreeBSD ioctl interfaces.
- can someone review and commit a patch from this issue: https://issues.asterisk.org/view.php?id=15908
By: Leif Madsen (lmadsen) 2009-11-11 11:05:00.000-0600

Can this issue be closed now that it appears we have a separate repo for DAHDI on FreeBSD now?
By: Max Khon (max khon) 2009-11-11 11:12:20.000-0600

Yes, please close it.
By: Leif Madsen (lmadsen) 2009-11-11 11:13:50.000-0600

Issue closed as this is now handled via the FreeBSD DAHDI port.