Hi,
Thanks for replying.
On 2/28/07, Jim Meyering <[EMAIL PROTECTED]> wrote:
Thanks for taking the time to work on this.
"Siavosh Benabbas" <[EMAIL PROTECTED]> 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
_______________________________________________
parted-devel mailing list
[email protected]
http://lists.alioth.debian.org/mailman/listinfo/parted-devel
