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 <[email protected]> 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
>
>
