On Friday 03 of August 2012 16:40:07 Denys Vlasenko wrote:
> 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"?
>
I tried to write a note about number 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;
