[PATCH v5 1/1] ldconfig: add --install option

[DJ Delorie]

Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
>>    { "format", 'c', N_("FORMAT"), 0, N_("Format to use: new (default), old, or compat"), 0},
>>    { "ignore-aux-cache", 'i', NULL, 0, N_("Ignore auxiliary cache file"), 0},
>> +  { "install", 'I', NULL, 0, N_("install pre-existing cache file"), 0},
>
> Maybe use capital letter for 'install', as previous lines.

Fixed.

>> +      /* This matches the temp file created by cache.c, and should be
>> +	 on the same filesystem as the cache file.  */
>> +      sprintf(dest, "%s~", cache_file);
>
> Missing space after function call, and I really think importing and using
> xasprintf will be better here.

Normally I would agree, but as I said before, the rest of ldconfig uses
a mix of xmalloc/alloca/sprintf and asprintf so switching everything to
xasprintf would be outside the scope of this change.  If I switch to
asprintf then I don't get the benefit of the xmalloc error handling.

>> +      char buf[512];
>> +      int r, w = 0, sz = 0;
>
> Maybe use ssize_t here, since it will compare against off_t later from
> stat return code.

Fixed.

>> +      /* At this point, sz contains the number of bytes copied so far.
>> +	 Copy the rest of the file.  */
>> +      while ((r = read (src_fd, buf, sizeof(buf))) > 0)
>> +	{
>> +	  bp = buf;
>> +	  while (r > 0 && (w = write (dest_fd, bp, r)) > 0)
>> +	    {
>> +	      bp += r;
>
> I think it should be like the first loop and advance the 'bp' by the written count,
> instead of the remaining count. Or at least assert that 'r == w' and fail otherwise.

Should be w, fixed.

> Also, a write error (r < 0) might reported with a stale errno. I think it would be
> better to mirror what you did on the loop above.

I put more error checking there.

>> +	}
>> +
>> +      fsync (dest_fd);
>
> Also check the fsync returned code, as elf/cache.c:730.

Done.

>> +      close (dest_fd);
>
> Maybe also check close returned here, although I am not sure if this is really
> required (save_cache does that).

Done anyway.