[parted-devel] porting to FreeBSD

[Debarshi 'Rishi' Ray]

> Thanks for your time.

It is a pleasure. :-)

+	freebsd* | kfreebsd*-gnu)
+			OS=freebsd ;;
 	*)	AC_MSG_ERROR([Unknown or unsupported OS "$host_os".  Only
"linux", "gnu" and "beos" are supported in this version of GNU
Parted.]) ;;

In the above snippet, can we have '*freebsd*' instead of 'freebsd* |
kfreebsd*-gnu'?
In the AC_MSG_ERROR message we should include 'freebsd' too.

> 1) I saw your patch after sending mine. I am not sure if it compiles
> on FreeBSD as sigaction is defined there but is not used. I will check
> this and simplify my patch if possible.

Are you referring to the following?
+if test "$OS" != freebsd; then
+   AC_CHECK_FUNCS(sigaction)
+fi
I do not think we should forbid the use of 'sigaction' if it is
available. Any particular reason other than the missing constants for
you to do this?

In freebsd.h:
+    libparted - a library for manipulating disk partitions
+    Copyright (C) 2001 Free Software Foundation, Inc.
Should it not be 2007 instead of 2001 in the copyright notice?

In freebsd.c:
+    libparted - a library for manipulating disk partitions
+    Copyright (C) 1999 - 2005 Free Software Foundation, Inc.
Should it not be 2007 instead of 199-2005 in the copyright notice?

You are following a mixed indentation style within the #ifdef...#endif
blocks. eg.,
+#if !defined(__FreeBSD_version) && defined(__FreeBSD_kernel_version)
+#define __FreeBSD_version __FreeBSD_kernel_version
+#endif
and then,
+#if ENABLE_NLS
+#  include <libintl.h>
+#  define _(String) dgettext (PACKAGE, String)
+#else
+#  define _(String) (String)
+#endif /* ENABLE_NLS */
Although it is true that after we have reformatted the entire code
using some tool (GNU Indent or Astyle) it will not matter, it is nice
to have any new code nicely formatted. I two space indentation within
the #ifdef...#endif blocks look nice to me (as in the second example).

In freebsd.c I find some mixed style of using braces around if...else branches.
+	if(np == NULL)
+	{
+		dev->type = PED_DEVICE_UNKNOWN;
+		return 0;
+	}
Should not we have the following?
        if (np === NULL) {
                dev->type = PED_DEVICE_UNKNOWN;
                return 0;
        }
I used the 8 space indentation since that is the most common
hereabouts. Maybe that will change in future, but till then lets
continue it.

Same here too:
+	if(strncmp(np, "ad", 2) == 0)
+	{
+		dev->type = PED_DEVICE_IDE;
and in a few more places.

+	FreeBSDSpecific* arch_specific = FREEBSD_SPECIFIC (dev);
+	off_t bytes = 0;
Maybe you can align all the variable names in a straight vertical
line. This is the style followed in most cases in the existing code.

Can this be moved to the beginning of freebsd.c?
+#if (__FreeBSD_version >= 500000) && (__FreeBSD_version < 600000)
+# define freebsd_get_ata_params freebsd_get_ata_params_compat5
+#elif (__FreeBSD_version >= 600000)
+# define freebsd_get_ata_params freebsd_get_ata_params_compat6
+#else
+# error "parted currently compiles on FreeBSD 5.X and 6.X only"
+#endif
If you mean to say that Parted only compiles on FreeBSD 5.x and 6.x,
then shouldn't this be something like the following?
#if (__FreeBSD_version >= 500000) && (__FreeBSD_version < 600000)
#  define freebsd_get_ata_params freebsd_get_ata_params_compat5
#elif (__FreeBSD_version >= 600000) && (__FreeBSD_version < 700000)
#  define freebsd_get_ata_params freebsd_get_ata_params_compat6
#else
#  error "parted currently compiles on FreeBSD 5.X and 6.X only"
#endif
Moreover can we not use function pointers instead of a macro to make
freebsd_get_ata_params point to freebsd_get_ata_params_compat5 or
freebsd_get_ata_params_compat6? A stronger type-checking provided by a
function pointer is better than a macro and would help in catching
mistakes.

+static int freebsd_get_ata_params_compat5(PedDevice *dev,
+										  struct ata_params *ap)
This should be:
static int
freebsd_get_ata_params_compat5 (PedDevice *dev, struct ata_params *ap)
Note the blankspace between the function name and the opening
parenthesis, and after the comma.

+	if (ioctl(fd, IOCATA, &iocmd) == -1)
+	{
+		ped_exception_throw (
+			PED_EXCEPTION_ERROR,
+			PED_EXCEPTION_CANCEL,
+			_("Could not get ATA parameters. Please"
+			  "contact the maintainer."));
+
+		ret = -1;
+		goto out;
+	}

I do not like this error message. How does one contact the maintainer?
Maintainer of which program? Instead of the maintainer should we not
point to somewhere like bug-parted_AT_gnu_DOT_org?

+#elif __FreeBSD_version >= 600000
+
+static int freebsd_get_ata_params_compat6(PedDevice *dev,
+										  struct ata_params *ap)
This should be:
#elif (__FreeBSD_version >= 600000) && (__FreeBSD_version < 700000)

static int
freebsd_get_ata_params_compat6 (PedDevice *dev, struct ata_params *ap)

Non uniform formatting in:
+		switch (ex_status) {
+		case PED_EXCEPTION_CANCEL:
+			goto error_close_dev;
+
+		case PED_EXCEPTION_UNHANDLED:
+			ped_exception_catch ();
+		case PED_EXCEPTION_IGNORE:
+			dev->model = strdup(_("IDE"));
+		}

Non uniform formatting:
+		switch (ex_status) {
+			case PED_EXCEPTION_CANCEL:
+				goto error_close_dev;
+
+			case PED_EXCEPTION_UNHANDLED:
+				ped_exception_catch ();
+			case PED_EXCEPTION_IGNORE:
+				; // just workaround for gcc 3.0
+		}
Also it would be a good idea to stick to pure C style comments using
/*....*/. A number of switch...case blocks after this onehave an odd
newline (or do not have a newline) between different cases.

Same problem as before, and can we not merge this with the previous block?
+#if (__FreeBSD_version >= 500000) && (__FreeBSD_version < 600000)
+# define _flush_cache freebsd_flush_cache_compat5
+#elif (__FreeBSD_version >= 600000)
+# define _flush_cache freebsd_flush_cache_compat6
+#else
+# error "parted currently compiles on FreeBSD 5.X and 6.X only"
+#endif

Can the following be also merged with the previous block?
+#if (__FreeBSD_version >= 500000) && (__FreeBSD_version < 600000)
+
+static void
+freebsd_flush_cache_compat5 (PedDevice* dev)

Same problem as before:
+#elif __FreeBSD_version >= 600000
Should be:
#elif (__FreeBSD_version >= 600000) && (__FreeBSD_version < 700000)

Lets just have pure C style comments:
+			_("kern.geom.conftxt sysctl not available, giving up!"));//Was printf
This is applicable to a number of other places too.

In libparted/labels/bsd.c the following:
 #include "config.h"
has changed to:
#include <config.h>
Same in libparted/labels/sun.c too.

Happy hacking,
Debarshi
-- 
GPG key ID: 63D4A5A7
Key server: pgp.mit.edu