[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


From: Samuel Thibault
Subject: Re: GSoC: PATH_MAX
Date: Fri, 8 Apr 2011 01:24:25 +0200
User-agent: Mutt/1.5.12-2006-07-14


Patrik Olsson, le Wed 06 Apr 2011 19:02:14 +0200, a écrit :
> As a preparation exercise for GSoC, I have fixed the PATH_MAX issue in
> three Debian packages: nekobee, whysynth and libsepol. There is no real
> reason I picked these packages, they were just first in the list here:
> https://buildd.debian.org/stats/?arch=hurd-i386&state=Failed

Could you rather look at the top of this page?


This is what is mostly needed. A lot of them will however not be
trivial so you'll have to sort it out a bit by checking what kind of
failure is happening in the Failed page (lockf, SA_SIGINFO, ghc6, etc.
are definitely difficult, for instance).

> Note that they are currently untested.

They should be tested before submitting, yes.

> Also note that I did not take the opportunity to fix unrelated bugs,
> such as in the cases where the code does not check the return value of
> snprintf.

I'd say don't expand the scope of your patch too much indeed. "Fixing
unrelated bugs" refers to the precise lines you already patch anyway.

> In those cases, I also just
> ignore the return value of asprintf, since I don't know what would be
> the appropriate way to handle such error anyway.

As antrik said, it depends on the situation. See what the code does for
other error cases, and just do the same.

> -            char buffer[PATH_MAX];
> -            snprintf(buffer, PATH_MAX, "%s/", project_directory);
> +            gchar *buffer = g_strdup_printf ("%s/", project_directory);
> gtk_file_selection_set_filename(GTK_FILE_SELECTION(file_selection),
>                                              buffer);
> +            g_free (buffer);

Looks good, please submit.

> -    snprintf(patches_tmp_filename, PATH_MAX, "%s/WhySynth_patches-%s",
> dir, path);
> +    if (patches_tmp_filename != NULL)
> +        g_free(patches_tmp_filename);
> +    patches_tmp_filename = g_strdup_printf ("%s/WhySynth_patches-%s",
> dir, path);

Looks good, please submit.

> -     snprintf(path, sizeof path, "%s/local.users", usersdir);
> +     asprintf(&path, "%s/local.users", usersdir);

As Antrik said, upstream may not be happy to depend on the presence of
asprintf. In that particular case, libsepol, the package is actually
expected to be used on Linux only, so it's probably fine. Now, since
it's actually Linux-only, it's probably also not really useful to port
this one...

>        err:
> +        free(path);

Please take care to keep the same indentation style as upstream uses.

You'll find a lot of various styles. Stick to everyone of them, to make
patch acceptance better.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]