Re: cpio improvements and fixes

[Shreenidhi Shedi]

Thanks Sergey for the feedback and review. Following are my answers for your questions and comments.

> [PATCH 01/12] Add header guard to all header files> The header files are local to the project and there's no possibility of
> any of them being included twice.  The '#ifdef' sentinels are thus
> useless.

Shedi: Agree with you. But we might include a header in a .h file and include both headers in a C file. This is just an attempt to prevent that situation. Just being defensive here.

> [PATCH 03/12] Format specifier fixes
> Needs more investigation (the use of %zu is not portable)

Shedi: Shall I just do something like this: fptintf(stderr, "nlink = %u ...", (unsigned) h->c_nlink, ...); instead?

> [PATCH 05/12] Enclose sizeof parameter in braces
> Purely aesthetic change and, as such, arguable.  In my opinion, redundant
> braces harm readability.

Shree: Agree. It's your call, I'm not sure about the coding standard followed in cpio. I'm comfortable with having braces with sizeof.

> [PATCH 07/12] Optimize ds_resize logic
> That's wrong.  The fact that x2nrealloc succeeded means only that
> the allocated block is ~50% bigger than the previously used one,
> but this does not imply its size is sufficient to accomodate len
> bytes.  Hence the need for the loop.

Shedi: In that case can we do,
void ds_resize (dynamic_string *string, size_t len)
{
  len += string->ds_idx;

  if (len >= string->ds_size)
  {
      len -= string->ds_size;
      if (len == 0)
      {
          len = 1;
      }
      else
      {
          len <<= 1;
      }
      string->ds_string = x2nrealloc (string->ds_string, &string->ds_size,
                                      len);
  }
}

> [PATCH 08/12] Reformat & refactor userspec.c
> What's the purpose of this?

Shedi: Alloca is a deprecated function in C99. Static analyzers throw a warning for alloca usage. So, I started modifying code to remove alloca usage and ended up refactoring it.

> [PATCH 09/12] Use destination size in strncpy to avoid
> 'stringop-overflow' warning from gcc
> tar_hdr->name is TARNAMESIZE bytes wide.  The copying is protected by
> 'if (name_len <= TARNAMESIZE)', which rules out any possibility of
> overflow.

Shedi: Agree with you here and the code is correct, gcc throws this warning. I tried to keep gcc happy and suppress that warning.
My changes are appropriate and the original version is right too but gcc doesn't like it.

> [PATCH 11/12] Use strtol instead of atoi

> The patch contains lots of whitespace changes, which makes it hard to
> see the actual purpose.  Nevertheless:

> -      io_block_size = atoi (arg);
> +      io_block_size = (int) strtol (arg, NULL, 10);

Shedi: The next line has a `if (io_block_size < 1)` that should suffice right?

Thanks,

Shedi

On Fri, Sep 3, 2021 at 10:57 AM Sergey Poznyakoff <gray@gnu.org.ua> wrote:

Hi Shedi,

Thanks for your mail.  Below I'm addressing each patch separately:

> [PATCH 01/12] Add header guard to all header files

The header files are local to the project and there's no possibility of
any of them being included twice.  The '#ifdef' sentinels are thus
useless.

> [PATCH 02/12] Remove ^L from files & redundant empty lines

The linefeed characters are inserted on purpose and should not be
removed.

> [PATCH 04/12] Use the safer alternative snprintf instead of sprintf

Fixed in 4d169305dc.

> [PATCH 03/12] Format specifier fixes

Needs more investigation (the use of %zu is not portable)

> [PATCH 05/12] Enclose sizeof parameter in braces

Purely aesthetic change and, as such, arguable.  In my opinion, redundant
braces harm readability.

> [PATCH 06/12] Remove redundant condition check

Applied (86dacfe3e0)

> [PATCH 07/12] Optimize ds_resize logic

That's wrong.  The fact that x2nrealloc succeeded means only that
the allocated block is ~50% bigger than the previously used one,
but this does not imply its size is sufficient to accomodate len
bytes.  Hence the need for the loop.

> [PATCH 08/12] Reformat & refactor userspec.c

What's the purpose of this?

> [PATCH 10/12] Optimize cpio_realloc_c_name logic

Wrong.  See comment to 07/12.

> [PATCH 09/12] Use destination size in strncpy to avoid
> 'stringop-overflow' warning from gcc

tar_hdr->name is TARNAMESIZE bytes wide.  The copying is protected by
'if (name_len <= TARNAMESIZE)', which rules out any possibility of
overflow.

> [PATCH 11/12] Use strtol instead of atoi

The patch contains lots of whitespace changes, which makes it hard to
see the actual purpose.  Nevertheless:

> -      io_block_size = atoi (arg);
> +      io_block_size = (int) strtol (arg, NULL, 10);

This has little sense without proper error checking.

> [PATCH 12/12] Use void where functions don't take any arguments

That's reasonable.  I'll apply this.

Regards,
Sergey