duplicity-team team mailing list archive

[Aaron Whitehouse <lists@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> 
> === modified file 'duplicity/selection.py'
> --- duplicity/selection.py	2015-07-31 08:22:31 +0000
> +++ duplicity/selection.py	2015-08-03 18:32:04 +0000
> @@ -151,16 +151,7 @@
>              for filename in robust.listpath(path):
>                  new_path = robust.check_common_error(
>                      error_handler, Path.append, (path, filename))
> -                # make sure file is read accessible
> -                if (new_path and new_path.type in ["reg", "dir"]
> -                        and not os.access(new_path.name, os.R_OK)):
> -                    log.Warn(_("Error accessing possibly locked file %s") % util.ufn(new_path.name),
> -                             log.WarningCode.cannot_read,
> -                             util.escape(new_path.name))

If you are taking this check out here, where are you adding it back in? I saw your comment above that "[t]he preferred way to see if you can read from a file is to attempt to open it for reading, so instead we just catch the exception later on when we try to use the path", but where are you doing that? The change below to exclude_sel_func around line 447 appears to be in the present_get_sf function, which is a relatively rare function that will not affect any "normal" backup commands (e.g. using --includes or --include-file-list). I haven't looked into this proposed merge in detail, but from memory the select functions do not check file existence/access (with the one exception of checking files ending in / are a dir, which is taken from path.isdir()) and assume that they will only be passed files (from diryield etc) that exist and are accessible. If you take away this check here, I'm not sure where (if anywhere) the lack of access will be caught. The two areas I would want to look at closely if I were you would be the directory listing and traversal in diryield etc and, if you are not catching access issues there, where other parts of duplicity actually use the list of files to backup etc. Another warning is that I don't remember seeing many tests in the test suite testing behaviour on locked files, so Here Be Dragons!

> -                    if diffdir.stats:
> -                        diffdir.stats.Errors += 1
> -                    new_path = None
> -                elif new_path:
> +                if new_path:
>                      s = self.Select(new_path)
>                      if s == 1:
>                          yield (new_path, 0)


-- 
https://code.launchpad.net/~jmwilson/duplicity/filecaps/+merge/266773
Your team duplicity-team is requested to review the proposed merge of lp:~jmwilson/duplicity/filecaps into lp:duplicity.