Re: PATCH 1/2 - fix all compiler warnings. (was XMLFS for GSoC)

[Thomas Schwinge]

Hallo!

Olaf surely will already be drafting the email to introduce you to the
proper Git patch submissing wonders (short: have a local branch, better
yet, a separte branch per topic, commit logically independent patches in
there (for example, adding a .gitignore file is different from fixing
compiler warnings), then use git format-patch and / or git send-email to
submit these), but here a few comments already.


On Mon, 4 Apr 2011 13:35:21 +0100, Michael Walker <mike@barrucadu.co.uk> wrote:
> The below patch fixes all compiler warnings.

That's always a good start!  Where is this repository that your patch is
against?

In case that you should ever consider valid warnings in core Hurd / GNU
Mach / glibc header files, these are worth being fixed properly, instead
of being worked around in client code (translators, etc.).  We're aware
that the packages themselves don't build without (a lot of...) warnings,
too, and that should be worked on, too, but is not related to your
project here, of course.

A lot of your changes are obviously fine, here are some additional
comments.


> It's quite long as I use
> a lot of warning flags (habit from when I was working on my own kernel
> project):

Hmm...

> diff --git a/Makefile b/Makefile
> index 3c66000..d646475 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,6 +1,7 @@
>  CC = gcc
> -CFLAGS = `pkg-config libxml-2.0 --cflags` -Wall -ggdb3 -O0 -std=c99
> -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE
> -COMPILE=$(CC) $(CFLAGS)
> +CFLAGS = `pkg-config libxml-2.0 --cflags` -ggdb3 -O0 -std=c99
> -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE

Your editor / mailer broke the lines, so this patch won't even apply.
Either you instruct your editor / mailer, or let git send-email take care
of that, or attach the patches instead of publishing them inline.

> +WARN = -Wall -Wextra -Wshadow -Wpointer-arith -Wcast-align
> -Wwrite-strings -Wmissing-declarations -Wredundant-decls
> -Wnested-externs -Winline -Wno-long-long -Winit-self
> -Wmissing-prototypes -Wstrict-prototypes -Wconversion -pedantic

Hrm.  Not sure whether we'd like all of these.  But you're (to the best
of my knowledge) to only person working on xmlfs these days, and we have
no general template, so I'd let you decide.

> diff --git a/netfs.c b/netfs.c
> index e23fe08..b95c908 100644
> --- a/netfs.c
> +++ b/netfs.c
> @@ -42,6 +43,9 @@ FILE *debug;
>  error_t
>  netfs_validate_stat (struct node *np, struct iouser *cred)
>  {
> +  (void) np;
> +  (void) cred;

If we really want this, wouldn't it be better to use ``__attribute__
((unused))'' with each of the functions' parameters?  In Hurd code, we
can unconditionally use GNU C / GCC features.


> @@ -336,7 +433,7 @@ error_t netfs_attempt_read (struct iouser *cred,
> struct node *node,
>    if (offset < size)
>      {
>        DEBUG ("INFO: copying the node\n");
> -      int gsize = size;
> +      int gsize = (int) size;

I don't know the surrounding code yet, but should gsize perhaps by a
size_t instead?

> @@ -346,7 +443,7 @@ error_t netfs_attempt_read (struct iouser *cred,
> struct node *node,
> 
>        /* Adding newline for user's convenience. */
>        if (offset + size == gsize)
> -       memcpy (data + size++, "\n", 1);
> +       memcpy ((void*) ((size_t) data + size++), "\n", 1);

memcpy with length of one?  Why not replace that with:

    data[size++] = '\n';

(Your cast of data doesn't look right anyway; do you understand and
agree?)  I should have a look at the whole function -- it still looks a
bit suspicious: what will come after the '\n' charater; is a '\0'
expected there?

> +/* Add the length of a directory entry for NAME to SIZE and return true,
> +   unless it would overflow MAX_DATA_LEN or NUM_ENTRIES, in which case
> +   return false.  */
> +int
> +bump_size (const char *name, int num_entries, int *count,
> +           size_t *size, vm_size_t max_data_len)
> +{
> +  [...]
> +}
> +
> +int
> +add_dir_entry (const char *name, ino_t thefileno, int type, struct node *dir,
> +               int num_entries, int *count, size_t size, char **p)
> +{
> +  [...]
> +}

> @@ -425,26 +594,10 @@ netfs_get_dirents (struct iouser *cred, struct node 
> *dir,
>    size_t size = 0;
>    error_t err;
> 
> -  /* Add the length of a directory entry for NAME to SIZE and return true,
> -     unless it would overflow MAX_DATA_LEN or NUM_ENTRIES, in which case
> -     return false.  */
> -  int bump_size (const char *name)
> -    {
> -      [...]
> -    }

> -      int add_dir_entry (const char *name, ino_t fileno, int type)
> -     {
> -       [...]
> -     }
> -

Why move these functions?  Typically, all definitions should be as tight
in scope as possible.

> diff --git a/xmlfs.c b/xmlfs.c
> index ced136d..bd39dec 100644
> --- a/xmlfs.c
> +++ b/xmlfs.c
> @@ -30,10 +30,13 @@
> 
>  FILE *debug;
> 
> -char *netfs_server_name = "xmlfs";
> -char *netfs_server_version = XMLFS_VERSION; /* defined in version.h */
> +char *netfs_server_name = (char*) "xmlfs";
> +char *netfs_server_version = (char*) XMLFS_VERSION; /* defined in version.h 
> */

That doesn't look sane.  According to netfs.h these indeed aren't const,
but why?  They're used only in libnetfs/io-version.c.

>  const char *argp_program_version;
> 
> +/* The filename of the XML */
> +char *xmlfilename = NULL;
> +
>  /* our filesystem */
>  struct xmlfs *xmlfs;
> 
> @@ -52,37 +54,39 @@ static const char doc[] =
>    "\vThis translator appears like a directory which tries to match the XML"
>    " tree in XML-DOC as closely as possible.";
> 
> +error_t parse_opt (int key, char *arg, struct argp_state *state)
> +{
> +  [...]
> +}
> +
>  int
>  main (int argc, char **argv)
>  {
>    mach_port_t bootstrap, underlying_node;
>    io_statbuf_t underlying_stat;
>    file_t xmlfile;
> -  char *xmlfilename = NULL;
>    error_t err;
> 
> +  xmlfilename = NULL;
>    debug = NULL;
> 
> -  error_t parse_opt (int key, char *arg, struct argp_state *state)
> -    {
> -      [...]
> -    }

As before; why?

> @@ -104,9 +108,9 @@ main (int argc, char **argv)
>    if (!xmlfilename)
>        /* Try to open the underlying node, which is incidently
>        our default XML file. */
> -    xmlfile = openport (underlying_node, O_READ);
> +    xmlfile = (file_t) openport (underlying_node, O_READ);
>    else
> -    xmlfile = open (xmlfilename, O_READ);
> +    xmlfile = (file_t) open (xmlfilename, O_READ);

Both openport and open return an int file descriptor, so why is xmlfile a
file_t (which is another name for a mach_port_t)?

> @@ -114,7 +118,7 @@ main (int argc, char **argv)
> 
>    netfs_root_node->nn_stat = underlying_stat;
>    netfs_root_node->nn_stat.st_mode =
> -    S_IFDIR | (underlying_stat.st_mode & ~S_IFMT & ~S_ITRANS);
> +    S_IFDIR | (underlying_stat.st_mode & (unsigned int) ~S_IFMT &
> (unsigned int) ~S_ITRANS);

What's the warning here?


Grüße,
 Thomas

pgp_50y_cKqJK.pgp
Description: PGP signature