On 08/01/2012 03:32 PM, Jakub Filak wrote:
> * the old implementation checks only presence of the time file
> * now we check validity of the time file
>
> Signed-off-by: Jakub Filak <[email protected]>
> ---
> src/lib/dump_dir.c | 58
> +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c
> index eef01f7..303954c 100644
> --- a/src/lib/dump_dir.c
> +++ b/src/lib/dump_dir.c
> @@ -109,6 +109,58 @@ static bool exist_file_dir(const char *path)
> return false;
> }
>
> +/* Returns true if the file is not readable or */
> +/* if the file doesn't contain valid unixt time stamp */
> +/* implementation is a compromise between efficiency and accuracy */
> +/* the function gets false negative results for big numbers */
> +static bool not_valid_time_file(const char *filename)
> +{
> + /* Open input file, and parse it. */
> + int fd = open(filename, O_RDONLY);
> + if (fd < 0)
> + {
> + VERB2 perror_msg("Can't open '%s'", filename);
> + return true;
> + }
> +
> + /* ~ maximal number of digits for positive time stamp string*/
> + char time_buf[sizeof(time_t) * 3 + 1];
> + const ssize_t rdsz = read(fd, time_buf, sizeof(time_buf));
> +
> + /* Just reading, so no need to check the returned value. */
> + close(fd);
> +
> + if (rdsz == -1)
Out of paranoia, I usually do (rdsz < 0) check instead.
> + {
> + VERB2 perror_msg("Can't read from '%s'", filename);
> + return true;
> + }
> +
> + /* approximate maximal number of digits in timestamp is sizeof(time_t)*3
> */
> + /* buffer has this size + 1 byte for trailing '\0' */
> + /* if whole size of buffer was read then file is bigger */
> + /* than string representing maximal time stamp */
> + if (rdsz == sizeof(time_buf))
> + {
> + VERB2 log("File '%s' is too long to be valid unix "
> + "time stamp (max size %zdB)", filename, sizeof(time_buf));
> + return true;
> + }
> +
> + time_buf[rdsz] = '\0';
I would do this instead:
/* Our tools don't put trailing newline into time file,
* but we allow such format too:
*/
if (rdsz > 0 && time_buf[rdsz - 1] == '\n')
rdsz--;
time_buf[rdsz] = '\0';
> +
> + /* range should be OK because of file size condition above */
What does this comment mean? "range"?
> + /* hence check if the string consists only from digits */
> + if (!isdigit_str(time_buf))
> + {
> + VERB2 log("File '%s' doesn't contain valid unix "
> + "time stamp ('%s')", filename, time_buf);
> + return true;
> + }
> +
> + return false;
> +}
> +
> /* Return values:
> * -1: error (in this case, errno is 0 if error message is already logged)
> * 0: failed to lock (someone else has it locked)
> @@ -203,7 +255,7 @@ static int dd_lock(struct dump_dir *dd, unsigned
> sleep_usec, int flags)
> if (sleep_usec == WAIT_FOR_OTHER_PROCESS_USLEEP) /* yes */
> {
> strcpy(lock_buf + dirname_len, "/"FILENAME_TIME);
> - if (access(lock_buf, F_OK) != 0)
> + if (not_valid_time_file(lock_buf))
> {
> /* time file doesn't exist. We managed to lock the directory
> * which was just created by somebody else, or is almost deleted
> @@ -212,7 +264,7 @@ static int dd_lock(struct dump_dir *dd, unsigned
> sleep_usec, int flags)
> */
> strcpy(lock_buf + dirname_len, "/.lock");
> xunlink(lock_buf);
> - VERB1 log("Unlocked '%s' (no time file)", lock_buf);
> + VERB1 log("Unlocked '%s' (no or corrupted time file)", lock_buf);
> if (--count == 0)
> {
> errno = EISDIR; /* "this is an ordinary dir, not dump dir" */
> @@ -303,7 +355,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags)
> && access(dir, R_OK) == 0
> ) {
> char *time_file_name = concat_path_file(dir, FILENAME_TIME);
> - if (access(time_file_name, R_OK) != 0)
> + if (not_valid_time_file(time_file_name))
> {
> dd_close(dd);
> dd = NULL;
--
vda
