Bug 57272

So there are two possibilities here:

* explicit permission was granted by the copyright holder to relicense
  under the GPL|AFL dual-license

* that permission was not granted

If permission was granted, then the LGPL header no longer matters and can be deleted.

If permission was not granted, then that file is not AFL-compatible, and will effectively have to be treated as licensed under the GPL only.

*-util-*.c are only used in dbus-daemon and associated tools (dbus-monitor etc.), not in the libdbus-1 shared library, so if we have to treat that file as GPL-only, I don't think that should be harmful for projects like LibreOffice/OpenOffice: if necessary, we can say "on Windows, libdbus-1 is GPL|AFL but dbus-daemon and the tools are GPL-only".

Ralf, you committed that file. Which parts of it are covered by the LGPL header? (Is it just the !defined(HAVE_DIRENT_H) block?) Do you have permission from Werner Almesberger to distribute this code under D-Bus' GPL|AFL dual-license?

(In reply to comment #1)
> *-util-*.c are only used in dbus-daemon and associated tools (dbus-monitor
> etc.), not in the libdbus-1 shared library

This was always meant to be true, and has always been true under the autotools build system (I think), but when built with CMake it was only made true in commit 301fa55 (between 1.5.8 and 1.5.10).

(In reply to comment #1)
> Ralf, you committed [dbus-sysdeps-util-win.c].
> Which parts of it are covered by the LGPL
> header? (Is it just the !defined(HAVE_DIRENT_H) block?) Do you have
> permission from Werner Almesberger to distribute this code under D-Bus'
> GPL|AFL dual-license?

Er, it'd help if I cc'd Ralf before asking him questions...

(See previous comments for context.)

(In reply to comment #3)
> (In reply to comment #1)
> > Ralf, you committed [dbus-sysdeps-util-win.c].
> > Which parts of it are covered by the LGPL
> > header? (Is it just the !defined(HAVE_DIRENT_H) block?) 

This belongs to the header which came from 
The header comes from http://www.trinitydesktop.org/docs/trinity/current/kdelibs/html/dirent_8h_source.html

> > Do you have
> > permission from Werner Almesberger to distribute this code under D-Bus'
> > GPL|AFL dual-license?

not requested, this has been added while porting dbus to windows under the hood of the KDE-Windows project. 

Please note that the related code is available under a bunch of licenses - I searched google for "Implement dirent-style opendir/readdir/closedir on Window 95/NT" and found

Apache license
http://www.msg.ucsf.edu/local/ganglia/ganglia-monitor-core-2.5.3/lib/readdir.c
http://www.msg.ucsf.edu/local/ganglia/ganglia-monitor-core-2.5.3/lib/readdir.h

PHP License https://github.com/moriyoshi/php-src/blob/master/LICENSE
https://github.com/moriyoshi/php-src/blob/master/win32/readdir.h
https://github.com/moriyoshi/php-src/blob/master/win32/readdir.c

gpl  (slightly modified implementation)
http://devel.squid-cache.org/changesets/squid/patches/10510.patch

Created attachment 105756 [details] [review]
COPYING: clarify licensing status of dbus-daemon on Windows

Might it be possible to fix the licensing problem instead of documenting it?

Gerv

(In reply to comment #6)
> Might it be possible to fix the licensing problem instead of documenting it?
> 
I see three ways to solve this issue

1. use one of the mentioned licenses if appliable otherwise
2. replace struct dirent and DIR by dbus own typedefs and use them in _dbus_opendir(), _dbus_readdir(), _dbus_closedir() or 
3. migrate directory and directory entry handling to a DBusDir and DBusDirEntry typedef and functions _dbus_dir_open(), _dbus_dir_read(), _dbus_dir_close() optional in a separate dbus_dir.c|h file

(In reply to comment #6)
> Might it be possible to fix the licensing problem instead of documenting it?

I don't consider it to be a problem that dbus-daemon.exe, when compiled for Windows, is effectively GPL-only. I do consider it to be a problem that it isn't clear that that is the case.

If you happen to have a permissively-licensed implementation of readdir()-style functionality that works on Windows, I'd be happy to review a patch.

Similarly, if you are unhappy with this functionality not being permissively-licensed, and you want to contact Werner Almesberger and ask for this to be relicensed under MIT/X11 or something, please go ahead.

If you're bundling dbus in LibreOffice but not distributing a Windows version of dbus-daemon, I suggest deleting that file, or replacing its contents with "/* deleted because it is GPL-only and unused */", in your copy.

Created attachment 105761 [details] [review]
Move LGPL parts of dbus-sysdeps-util-win.c into a separate  file

(In reply to comment #7)
> I see three ways to solve this issue
> 
> 1. use one of the mentioned licenses if appliable otherwise

This requires permission from the copyright holder. Do you know how to contact the copyright holder?

> 2. replace struct dirent and DIR by dbus own typedefs and use them in
> _dbus_opendir(), _dbus_readdir(), _dbus_closedir() or 
> 3. migrate directory and directory entry handling to a DBusDir and
> DBusDirEntry typedef and functions _dbus_dir_open(), _dbus_dir_read(),
> _dbus_dir_close() optional in a separate dbus_dir.c|h file

D-Bus *already* has its own cross-platform directory-listing API, namely _dbus_directory_open(), _dbus_directory_get_next_file(), _dbus_directory_close().

Reimplementing that API using Windows primitives without copying from a LGPL source, instead of going via a weird emulation of Unix APIs, would probably be a code-quality improvement, as well as allowing this LGPL code to be dropped. Patches welcome; I'm not going to write them, because I don't develop on Windows.

To be clear, D-Bus already defines a cross-platform directory-listing API; that is not the problem. 

The problem is that instead of implementing that directory-listing API using FindFirstFileA() or whatever, the author of this part of the D-Bus Windows support code found a LGPL implementation of the standard Unix opendir(), readdir(), closedir() functions in terms of Windows primitives, pasted it in, then implemented D-Bus' directory API in terms of those functions.

Hi Simon,

Is it possible to tell who the copyright holder is? Is it the Werner Almesberger you mentioned, or might there be more to it than that?

If this file is only used on Windows, then we could solve our issue (it shows up on license-checking scans) by just deleting it from our copy, but if we import a new version, it might come back, and it just seems easier to regularise the licensing of dbus. I'm happy to contact the necessary people, if someone else can do the source code management archaeology to tell me who I need to contact.

Gerv

(In reply to comment #12)
> Is it possible to tell who the copyright holder is? Is it the Werner
> Almesberger you mentioned, or might there be more to it than that?

I don't know any more than you do; I wasn't a D-Bus maintainer back when this was added. Hopefully Ralf knows - he committed it, back in the days of svn.

The relevant commit is

http://cgit.freedesktop.org/dbus/dbus/commit/?h=70bfc74e54ac8a9a93885710cd8350d1a58b3406

git blame says Marcus Brinkmann and Tor Lillqvist have also modified that chunk of code.

It is possible that some or all of these people agreed to MIT/X11 licensing for their contributions when Ryan Lortie was trying to get dbus relicensed to MIT/X11 (which fell through because the copyright on some old and rather fundamental bits was owned by CodeFactory AB, which went out of business, and nobody is sure who has ended up owning their assets, so nobody knows who can give permission to relicense their stuff).

Created attachment 105806 [details] [review]
Add directory test application

Created attachment 105807 [details] [review]
Port dbus_directory_function to use win32 native api function

As mentioned in comment 4 does the copyright issue came from the including of the header containing the DIR structure include, which is solved by the "port" patch

Comment on attachment 105807 [details] [review]
Port dbus_directory_function to use win32 native api function

Review of attachment 105807 [details] [review]:
-----------------------------------------------------------------

Thanks, I think this is the best approach to resolving this.

Looks good to me, except:

::: dbus/dbus-sysdeps-util-win.c
@@ +453,5 @@
>  
>    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>  
>    filename_c = _dbus_string_get_const_data (filename);
> +  filespec = dbus_malloc (strlen(filename_c) + 2 + 1);

Code below this point will crash if dbus_malloc() returns NULL here. _DBUS_SET_OOM (error) and return instead.

@@ +454,5 @@
>    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>  
>    filename_c = _dbus_string_get_const_data (filename);
> +  filespec = dbus_malloc (strlen(filename_c) + 2 + 1);
> +  strcpy (filespec, filename_c);

DBusString stops you needing to think about whether your strcat() has off-by-one errors.

This particular code looks OK at first glance; but I shouldn't have to even think about it, because if it used DBusString, it would be "obviously correct", which is the best sort of correct.

Created attachment 105809 [details] [review]
Improve debug message to be able to see empty files

convenience patch

Created attachment 105810 [details] [review]
Add directory test application (update 1)

unused label cleanup

Created attachment 105811 [details] [review]
Add dbus string convenience function

Created attachment 105812 [details] [review]
Port dbus_directory_function to use win32 native api function (update 1)

- use DBusString
- fix additional filename pattern case

(In reply to comment #17)
> Comment on attachment 105807 [details] [review] [review]
> Port dbus_directory_function to use win32 native api function
> 
> Review of attachment 105807 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Thanks, I think this is the best approach to resolving this.
> 
> Looks good to me, except:

should be solved now :-)

Can I assume that the updated patches are okay for commit to git master ?

(In reply to comment #23)
> Can I assume that the updated patches are okay for commit to git master ?

I would still like to review them properly, which I will do soon.

Comment on attachment 105809 [details] [review]
Improve debug message to be able to see empty files

Review of attachment 105809 [details] [review]:
-----------------------------------------------------------------

sure

Comment on attachment 105810 [details] [review]
Add directory test application (update 1)

Review of attachment 105810 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-errors.c
@@ +334,5 @@
>    return error->name != NULL;
>  }
>  
> +const char *
> +dbus_error_get_message (const DBusError *error)

Why is this needed? error->message is public API.

I don't think there's any point in having an accessor function here.

::: test/test-dir.c
@@ +1,1 @@
> +#include <config.h>

test-dir.c cannot be run as an automated test (it needs to be run with a command-line option, and its output needs human interpretation) so I'd prefer it to be named matching manual-* - perhaps manual-dir-iter.c or something.

@@ +3,5 @@
> +
> +#include "dbus/dbus-sysdeps.h"
> +
> +static void
> +die (const char *message)

(Not a merge blocker)

If you wrote it like this (requires dbus/dbus-macros.h to be included first), the compiler would understand that it cannot return:

static void die (const char *message) _DBUS_GNUC_NORETURN;

static void
die (const char *message)
{
  ...

@@ +30,5 @@
> +
> +  dbus_error_init (&tmp_error);
> +
> +  if (!_dbus_string_init (&filename))
> +      die ("memory allocation error");

(Not a merge blocker)

This is fine, but I often find that defining

    static void oom (void) _DBUS_GNUC_NORETURN;

adds clarity.

@@ +35,5 @@
> +
> +  if (!_dbus_string_init (&dirname))
> +      die ("memory allocation error");
> +
> +  _dbus_string_append (&dirname, argv[1]);

if (!_dbus_string_append (...))
  oom ();

(or die("..."))

@@ +39,5 @@
> +  _dbus_string_append (&dirname, argv[1]);
> +  dir = _dbus_directory_open (&dirname, &tmp_error);
> +
> +  if (dir == NULL)
> +      die ("could not open directory");

I'd personally do

if (dir == NULL)
  {
    fprintf (stderr, "could not open directory: %s: %s\n", tmp_error.name, tmp_error.message);
    exit (1);
  }

@@ +63,5 @@
> +      _dbus_string_free (&full_path);
> +    }
> +
> +  if (dbus_error_is_set (&tmp_error))
> +      die (dbus_error_get_message (&tmp_error));

You can just use die (tmp_error.message) here, but I'd personally do

if (dir == NULL)
  {
    fprintf (stderr, "could not open directory: %s: %s\n", tmp_error.name, tmp_error.message);
    exit (1);
  }

@@ +68,5 @@
> +
> +  _dbus_string_free (&filename);
> +
> +  if (dir)
> +    _dbus_directory_close (dir);

You don't _dbus_string_free (dirname), although it doesn't really matter, because you're about to exit anyway.

Comment on attachment 105811 [details] [review]
Add dbus string convenience function

Review of attachment 105811 [details] [review]:
-----------------------------------------------------------------

sure

Comment on attachment 105812 [details] [review]
Port dbus_directory_function to use win32 native api function (update 1)

Review of attachment 105812 [details] [review]:
-----------------------------------------------------------------

Looks OK, some minor adjustments that aren't merge blockers

::: dbus/dbus-sysdeps-util-win.c
@@ +455,3 @@
>      {
> +      dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> +                      "Could not allocate memory for directory filename copy");

(Not a merge blocker)

You could just use _DBUS_SET_OOM (error) or whatever the macro is called

@@ +462,5 @@
> +    {
> +      if (!_dbus_string_append (&filespec, "*"))
> +        {
> +          dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> +                          "Could not append filename wildcard");

same here

@@ +471,5 @@
> +    {
> +      if (!_dbus_string_append (&filespec, "\\*"))
> +        {
> +          dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> +                          "Could not append filename wildcard 2");

same here

The actual functional change looks good, thanks!

I would prefer some alterations to the manual test (your patch 2/4) before we merge that.

Comment on attachment 105756 [details] [review]
COPYING: clarify licensing status of dbus-daemon on Windows

obsoleted by Ralf's solution

Comment on attachment 105761 [details] [review]
Move LGPL parts of dbus-sysdeps-util-win.c into a separate  file

obsoleted by Ralf's solution

Comment on attachment 105809 [details] [review]
Improve debug message to be able to see empty files

committed to master

Comment on attachment 105811 [details] [review]
Add dbus string convenience function

committed to master

Thanks so much for solving this :-)

Gerv

Created attachment 105893 [details] [review]
Add directory test application (update 2)

(In reply to comment #28)
> Comment on attachment 105812 [details] [review] [review]
> Port dbus_directory_function to use win32 native api function (update 1)
> 
> Review of attachment 105812 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks OK, some minor adjustments that aren't merge blockers
> 
> ::: dbus/dbus-sysdeps-util-win.c
> @@ +455,3 @@
> >      {
> > +      dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> > +                      "Could not allocate memory for directory filename copy");
> 
> (Not a merge blocker)
> 
> You could just use _DBUS_SET_OOM (error) or whatever the macro is called
> 
> @@ +462,5 @@
> > +    {
> > +      if (!_dbus_string_append (&filespec, "*"))
> > +        {
> > +          dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> > +                          "Could not append filename wildcard");
> 
> same here
> 
> @@ +471,5 @@
> > +    {
> > +      if (!_dbus_string_append (&filespec, "\\*"))
> > +        {
> > +          dbus_set_error (error, DBUS_ERROR_NO_MEMORY,
> > +                          "Could not append filename wildcard 2");
> 
> same here

The drawback with using DBUS_SET_OOM is that the detailed error condition will get lost.

Comment on attachment 105893 [details] [review]
Add directory test application (update 2)

Review of attachment 105893 [details] [review]:
-----------------------------------------------------------------

ship it!

(In reply to comment #36)
> The drawback with using DBUS_SET_OOM is that the detailed error condition
> will get lost.

I don't think that really matters for OOM (if malloc() returns NULL, everything is ruined already, and dbus_set_error() will probably also hit an OOM condition and use a generic error message) but I do see your point.

OK to merge, with or without switching to _DBUS_SET_OOM().

(In reply to comment #37)
> Add directory test application (update 2)

I'd like this added to test/Makefile.am; you could do that (it's like manual-authz, except that it needs to be linked to libdbus-internal like test-syslog is), or I can do that if you'll review it.

Created attachment 105895 [details] [review]
Add test application 'manual-dir-iter' to autotools build system

(In reply to comment #40)
> Created attachment 105895 [details] [review] [review]
> Add test application 'manual-dir-iter' to autotools build system

Because I did not commit the base 'manual-dir-iter' patch, I can merge this into it, if you like.

(In reply to comment #41)
> Because I did not commit the base 'manual-dir-iter' patch, I can merge this
> into it, if you like.

Yes, please squash this in. It looks correct.

Comment on attachment 105893 [details] [review]
Add directory test application (update 2)

committed to master

Comment on attachment 105895 [details] [review]
Add test application 'manual-dir-iter' to autotools build system

committed to master

Comment on attachment 105812 [details] [review]
Port dbus_directory_function to use win32 native api function (update 1)

committed to master

Fixed in git for 1.9.0, then