[parted-devel] porting to FreeBSD

[Jim Meyering]

Thanks for taking the time to work on this.

"Siavosh Benabbas" <sbenabas at gmail.com> wrote:
> Thanks for the advice.
> I have cloned my version of git and commited the patch there. It is at
> http://www.cs.toronto.edu/~siavosh/parted-freebsd/
> Unfortunately directory listing is disabled on the server I don't know
> if I need to do anything special to make this into a git repository
> but I have placed a compressed version of the directory here:
> http://www.cs.toronto.edu/~siavosh/parted-freebsd.tar.gz
> I have removed the _GNU_SOURCE from the patch but can't compile here
> as I don't have gettext>=1.15.

Can't you just get and install the latest version of gettext?

> The patch is attached.

> diff --exclude=.git -NruBb parted/configure.ac parted-freebsd/configure.ac
> --- parted/configure.ac	Tue Feb 27 21:12:25 2007
> +++ parted-freebsd/configure.ac	Tue Feb 27 21:22:01 2007
> @@ -51,6 +51,8 @@
>  	linux*) OS=linux ;;
>  	gnu*)	OS=gnu ;;
>  	beos*)	OS=beos ;;
> +	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.]) ;;
>  esac
>  AC_SUBST(OS)
> @@ -184,6 +186,7 @@
>  	#include <sys/types.h>
>  	#include <unistd.h>
>  ])
> +AC_CHECK_TYPE(loff_t, long long)

Why do you want to define the loff_t type at this level?
Won't it cause conflicts with the existing definitions?

    libparted/fs/ext2/ext2.h:48:  typedef off_t loff_t;
    libparted/fs/xfs/platform_defs.h:61:typedef loff_t xfs_off_t;

What happens if it is not defined?
Can you fix it with a less-global change?

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

Please use a separate invocation of AC_CHECK_HEADERS,
and mention that these files are specific to FreeBSD.

> -AC_CHECK_HEADERS(getopt.h)
> +AC_CHECK_HEADERS(getopt.h endian.h sys/endian.h)
>
>  dnl required for libparted/llseek.c  (TODO: make linux-x86 only)
>  if test "$OS" = linux; then
> @@ -450,7 +467,9 @@

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

Whoa.  FreeBSD provides sigaction.
Why do you want to skip the test for it on that system?

> -AC_CHECK_FUNCS(sigaction)
> +if test "$OS" != freebsd; then
> +   AC_CHECK_FUNCS(sigaction)
> +fi
>  AC_CHECK_FUNCS(getuid)

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

The following change looks avoidable.
There should be a move to *remove* casts, not to add them,
especially, ones that look like that :)
I'll bet we can find a better way.

> -    signal (SIGFPE, &sigfpe_handler);
> +    signal (SIGFPE, (void (*) (int)) &sigfpe_handler);

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

Finally, on a superficial scan through the rest, I spotted an
obvious potential buffer overrun.  In _probe_standard_devices,
if the parsed devname or devtype is 16 bytes long, then that
use of sscanf will write a NUL byte past the end of the array.
This is a good reason (there are many others) to avoid using sscanf
in favor of functions like strtok and strtoul.

> diff --exclude=.git -NruBb parted/libparted/arch/freebsd.c parted-freebsd/libparted/arch/freebsd.c
...
> +static int
> +_probe_standard_devices ()
> +{
> +	char		devname [16];
> +	char		devtype [16];
...
> +	while (sscanf (cptr, "%*d %16s %16s",
> +				   devtype, devname) != EOF) {
...
> +			printf("Probing %s ...\n", fullpath);

Also, there are pretty many print statements in that file.
Shouldn't those be uses of ped_exception_throw instead?
I do see that there are is one printf call in each of the sibling
.c files, but those are used only #ifdef READ_ONLY.
In general, library functions must not generate output,
and certainly not to stdout.