[Parted-maintainers] Bug#754588: FTBFS with clang instead of gcc

[Colin Watson]

On Sat, Jul 12, 2014 at 11:34:18PM +0400, Alexander wrote:
> --- ./lib/regex_internal.c	2014-07-12 23:29:47.000000000 +0400
> +++ ../parted-2.3-my/./lib/regex_internal.c	2014-07-11 00:49:05.339625627 +0400
> @@ -1394,7 +1394,7 @@
>  internal_function
>  re_node_set_remove_at (re_node_set *set, Idx idx)
>  {
> -  if (idx < 0 || idx >= set->nelem)
> +  if (idx == 0 || idx >= set->nelem)
>      return;
>    --set->nelem;
>    for (; idx < set->nelem; idx++)

This seems like an incorrect transformation; while I don't know the
regex engine particularly well, this seems like the sort of code
structure where 0 is a perfectly reasonable value for idx to take
(indicating the first element).  Perhaps putting the less-than-zero
check within #ifndef _REGEX_LARGE_OFFSETS would be a better fix;
something like:

#ifndef _REGEX_LARGE_OFFSETS
  /* Idx is a signed type.  */
  if (idx < 0)
    return;
#endif
  if (idx >= set->nelem)
    return;

However, in general, suggested fixes to code imported from Gnulib should
be sent directly to Gnulib in the first instance rather than to
maintainers of code downstream from it
(https://www.gnu.org/software/gnulib/).  Otherwise (a) it's awkward to
maintain downstream and (b) you'll find yourself playing whack-a-mole
with the same problem in multiple projects.

If you've sent this patch to the same file in other projects, perhaps
you could pass on my review, because I'd be worried about this suggested
change causing subtle run-time errors.

> --- ./libparted/fs/fat/fat.c	2014-07-12 23:29:47.000000000 +0400
> +++ ../parted-2.3-my/./libparted/fs/fat/fat.c	2014-07-12 21:57:44.360150178 +0400
> @@ -805,73 +805,73 @@
>  #endif /* !DISCOVER_ONLY */
>  
>  static PedFileSystemOps fat16_ops = {
> -	probe:		fat_probe_fat16,
> +	.probe = fat_probe_fat16,
[etc.]

I agree it makes sense to switch to the more modern designated
initialiser syntax.  Would you mind rebasing this part of your patch on
top of the latest upstream git repository
(https://www.gnu.org/software/parted/) and sending it upstream?  It'll
probably be somewhat smaller due to the removal of a good deal of
filesystem code in Parted 3.0.  I'll be switching to a new upstream
version soon (https://bugs.debian.org/754582), and don't have any plans
to upload anything else based on 2.3, so this shouldn't slow things down
much.

> --- ./libparted/debug.c	2010-02-08 09:48:18.000000000 +0300
> +++ ../parted-2.3-my/./libparted/debug.c	2014-07-12 23:24:00.045547358 +0400
> @@ -106,7 +106,7 @@
>          /* Throw the exception */
>          ped_exception_throw (
>                  PED_EXCEPTION_BUG,
> -                PED_EXCEPTION_FATAL,
> +                PED_EXCEPTION_CANCEL,
>                  _("Assertion (%s) at %s:%d in function %s() failed."),
>                  cond_text, file, line, function);
>          abort ();

In general it's a good idea to send logically separate changes as
separate patches, rather than burying something like this in the middle
of a large and fairly mechanical patch for something different.

The previous code was indeed using a value from the wrong enumeration.
I wonder if it might not be slightly safer to transform this to
PED_EXCEPTION_NO instead, which has the same numerical value as
PED_EXCEPTION_FATAL; it probably won't matter much as ped_assert is
going to abort immediately afterwards, but it can go through a
user-specified exception handler on the way so this could possibly
change behaviour.  Again, I'd be more comfortable if you could send this
directly upstream so that they can sign off on it.

Thanks,

-- 
Colin Watson                                       [cjwatson at debian.org]