[parted-devel] porting to FreeBSD

[Siavosh Benabbas]

Hi,
Thanks for replying.

On 2/28/07, Jim Meyering <jim at meyering.net> wrote:
> 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?
Well it is a bit harder than that. A lot of things depend on gettext
and the latest version has not been added to FreeBSD's port
collection, I would like to avoid a build outside of that collection.
I will do this sometime soon and let you know.
>
> > 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?
>
> --------------------------
Aren't these filesystem specific headers? I think we need such a
definition for FreeBSD. I will give that a try.
>
> 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 @@
>
> --------------------------
>
Ok. Good point.
> 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)
>
> --------------------------
>

Well FreeBSD does provide sigaction but it misses (except for the
CURRENT (unstable) branch) many MACRO constants that you can check the
value of siginfo_t variables against. But I think that change in the
configure.ac is avoidable.

> 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);
>

I agree that there should be a move to remove the casts but only by
making the argument of correct type. Here we are using one
sigfpe_handler function for two cases:
1. The system has sigaction which takes a pointer to a function with
three arguments.
2. The system has signal which takes a pointer to a function with one arguments.

Now in any architecture that I know of functions with up to three
integer or pointer arguments pass their arguments in the registers so
this works. But according to a C compiler this generates a warning or
error depending on the situation. In FreeBSD without sigaction you
would have an error.

> --------------------------
>
> 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);
>
Agreed. Thanks for the comment.

> 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.
>
Agreed. I will do a grep printf and fix those.

Regards,
Siavosh Benabbas