Re: [Bug-tar] adding ACLs when there are none

2014-03-05 Thread Joerg Schilling
"Linda A. Walsh"  wrote:

>
>
> Pavel Raiskup wrote:
>
> > When --acls option is on (regardless of tarball contents or
> > tarball format), we should explicitly set OR delete default ACLs
> > for extracted directories.  Prior to this update, we always
> > created arbitrary default ACLs based standard file permissions.
> > 
> 
> Why would tar create any acls if there are none in the source tar?
>
> I saw someone else have a similar complaint about acls being created
> when the tar didn't have acls but the --acls option was used.

If gtar adds  ACLs to archives in case there are only the hisorical UNIX 
permissiond, it is buggy because it does not follow the definition
of the ACL enhancements.

On the extract side: I did already mention that gtar is buggy because it does 
not delete existing ACLs in case that a file was archives without ACLs.

Jörg

-- 
 EMail:jo...@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
   j...@cs.tu-berlin.de(uni)  
   joerg.schill...@fokus.fraunhofer.de (work) Blog: 
http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/private/ ftp://ftp.berlios.de/pub/schily



Re: [Bug-tar] adding ACLs when there are none

2014-03-05 Thread Linda Walsh



Joerg Schilling wrote:

er take --acls similarly to -p in this regard.



This does nit seem to be correct.

The BSD sgroup bit on directories only propagates to directories and not to 
files. The default acls propagate to files also.


Note that the bevior in star has been defined in 2001 after talking to various 
people. gtar should behave similar.
  


There some wiggle room here, I agree,
The patch as I understood it, removed the default acl on the directories.

The files only have the option of inheriting a default acl from the 
directory.


If the files from the tar put in the directory don't have acls attached 
from the
dir, because of --acls, I wouldn't be upset with that. (without --acls, 
I'd expect them
to inherit them). 


But without default acl's present in the tar (for the directories --
as separate from the standard acl), I'd expect them to be left alone. 

BTW, to be clear, when I say "expect", I mean "least surprise", not as a 
mandate
about what should be done.  Thus I could see --acls clearing everything 
except
the default acls on the directory -- meaning new files and dirs would 
inherit

as normal as before the tar.





Re: [Bug-tar] adding ACLs when there are none

2014-03-05 Thread Joerg Schilling
Pavel Raiskup  wrote:

> On Wednesday, March 05, 2014 05:06:06 Linda A. Walsh wrote:
> > Pavel Raiskup wrote:
> > > Or could you give an example?  What *exactly* do you expect the --acls
> > > should behave by default?  Combine existing acls in parent directory
> > > (default acls) with the stored in archive?
> > >
> > > Thanks, Pavel
> > >
> > -
>
> > If the SetGid bit is set on a directory on linux, it is usually
> > propagated to lower lower level dirs to permit a particular type of
> > access to be propagated to  lower level files and dirs.
>
> The _default_ seems to be matter of taste.  Looking at how the SetGid
> works in GNU tar, the bit is inherited from parent by default (no
> additional option passed).  But when you specify '-p' option, then the bit
> is not inherited as you want (the permissions stored in archive have a
> priority).  I would rather take --acls similarly to -p in this regard.

This does nit seem to be correct.

The BSD sgroup bit on directories only propagates to directories and not to 
files. The default acls propagate to files also.

Note that the bevior in star has been defined in 2001 after talking to various 
people. gtar should behave similar.

Jörg

-- 
 EMail:jo...@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
   j...@cs.tu-berlin.de(uni)  
   joerg.schill...@fokus.fraunhofer.de (work) Blog: 
http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/private/ ftp://ftp.berlios.de/pub/schily



Re: [Bug-tar] adding ACLs when there are none

2014-03-05 Thread Pavel Raiskup
On Wednesday, March 05, 2014 05:06:06 Linda A. Walsh wrote:
> Pavel Raiskup wrote:
> > Or could you give an example?  What *exactly* do you expect the --acls
> > should behave by default?  Combine existing acls in parent directory
> > (default acls) with the stored in archive?
> >
> > Thanks, Pavel
> >
> -

> If the SetGid bit is set on a directory on linux, it is usually
> propagated to lower lower level dirs to permit a particular type of
> access to be propagated to  lower level files and dirs.

The _default_ seems to be matter of taste.  Looking at how the SetGid
works in GNU tar, the bit is inherited from parent by default (no
additional option passed).  But when you specify '-p' option, then the bit
is not inherited as you want (the permissions stored in archive have a
priority).  I would rather take --acls similarly to -p in this regard.

> If a default acl is set on a dir I see it used in the same way.
> 
> Sorry, didn't finish that thought...If there are default acls set in the
> tar, they would replace such default acls that are present, but
> undefined ACLs in the tar wouldn't overwrite set acl's that propagate
> from the parent.

And what if you want to combine default ACLs somehow?

Also what if you want the inherited (not in archive-stored) ACLs to affect
also all extracted files?

I agree that you may want to specify exact behavior.  The --acls could
take optional arguments like --acls=inherit-defaults (your scenario).
Possibly --acls=combine (in future) to merge as much as possible (little
bit more work here would be needed).  Thoughts?

Pavel




Re: [Bug-tar] adding ACLs when there are none

2014-03-05 Thread Linda A. Walsh

Pavel Raiskup wrote:

Or could you give an example?  What *exactly* do you expect the --acls
should behave by default?  Combine existing acls in parent directory
(default acls) with the stored in archive?

Thanks, Pavel
  

-
Sorry, didn't finish that thought...If there are default acls set in the 
tar,

they would replace such default acls that are present, but undefined ACLs
in the tar wouldn't overwrite set acl's that propagate from the parent.





Re: [Bug-tar] adding ACLs when there are none

2014-03-05 Thread Linda Walsh



Pavel Raiskup wrote:

These are IMO candidates for omitting --acls option, no?

Or could you give an example?  What *exactly* do you expect the --acls
should behave by default?  Combine existing acls in parent directory
(default acls) with the stored in archive?

Leaving default acls in the dir -- case in in point...
  

If the SetGid bit is set on a directory on linux, it is usually propagated
to lower lower level dirs to permit a particular type of access to be
propagated to  lower level files and dirs.

If a default acl is set on a dir I see it used in the same way.





Re: [Bug-tar] seek_hole proposal [v2]

2014-03-05 Thread Pavel Raiskup
On Wednesday, March 05, 2014 12:14:17 Joerg Schilling wrote:
> Pavel Raiskup  wrote:
>
> > Note that after discussion [1] I still think that existing ST_IS_SPARSE
> > macro is better for file-sparseness detection than using SEEK_HOLE (not
> > worth having additional syscalls open~>seek~>close).
> 
> Your code is not compatible to the SEEK_HOLE interface. A file is sparse in 
> case that pathconf()/fpathconf(f, _PC_MIN_HOLE_SIZE) return a positive number 
> and lseek(f, (off_t)0, SEEK_HOLE) returns a number < stat.st_size.

Yes, apart from pathconf, the patch-v1 contained bug - I should react on
hole_offset, not data_offset.  And there was yet another bug - we need to
reposition the file offset to the beginning in this case (no FS support);
so there needs to be done this change:

   if (offset == 0 /* first loop */
-  && data_offset == st->stat.st_size)
-return false;
+  && data_offset == 0
+  && hole_offset == st->stat.st_size)
+{
+  lseek (fd, 0, SEEK_SET);
+  return false;
+}

Joerg, thanks for pointing that out.

> see "man pathconf":
> [...]
> 
> In other cases, the file stil may be sparse, but the filesystem does not 
> support SEEK_HOLE. I e.g. doubt that Linux correctly implements SEEK_HOLE
> for NFS.

Correct, Linux's NFS returns "just" the virtual hole at the end of sparse
file, which is now OK - we would still fallback to raw hole detection.
Fixed patch v2 attached.

Pavel
>From 0563cfd261f6c47f25924ae6fef542230fdf2794 Mon Sep 17 00:00:00 2001
From: Pavel Raiskup 
Date: Sun, 23 Feb 2014 13:12:54 +0100
Subject: [PATCH] tar: use SEEK_HOLE for hole detection

Reuse the SEEK_HOLE/SEEK_DATA feature of lseek when possible.
This makes the sparse file archivation to be quite faster as tar
does not need to perform additional read of whole file in order to
detect file sparse map.

This lseek feature is not yet fully POSIX but it is fairly widely
implemented these days.

Also implement --hole-detection option for proper method
selection.

* src/common.h (HOLE_DETECTION_RAW, HOLE_DETECTION_SEEK)
(HOLE_DETECTION_ALL, hole_detection): New constants and variable.
* src/sparse.c (sparse_scan_file_wholesparse): New function as a
method for detecting sparse files without any data.
(sparse_scan_file_raw): Renamed from sparse_scan_file, removed the
completely-sparse detection if-branch.
(sparse_scan_file_seek): Implements method for hole detection
using lseek.
(sparse_scan_file): Reimplemented function as a wrapper for all
methods.
* src/tar.c (HOLE_DETECTION_OPTION): New option.
(parse_opt): Handle new --hole-detection option.
* tests/sparse02.at: Use --hole-detection=raw as the seek method
creates little bit bigger archives causing test to fail.
* tests/checkseekhole.c: SEEK_HOLE detection helper.
* tests/sparsemv.at: Likewise.
* tests/sparsemvp.at: Likewise.
* tests/sparse05.at: New test-case.
* tests/testsuite.at: Cover new testcase.
* tests/Makefile.am: Likewise.
* doc/tar.1: Document.
* doc/tar.texi: Likewise.
---
 doc/tar.1 |   6 ++
 doc/tar.texi  |  70 +++-
 src/common.h  |   6 ++
 src/sparse.c  | 173 --
 src/tar.c |  16 +
 tests/Makefile.am |   5 +-
 tests/checkseekhole.c |  90 ++
 tests/sparse02.at |   2 +-
 tests/sparse05.at |  56 
 tests/sparsemv.at |   1 +
 tests/sparsemvp.at|   1 +
 tests/testsuite.at|  16 +
 12 files changed, 392 insertions(+), 50 deletions(-)
 create mode 100644 tests/checkseekhole.c
 create mode 100644 tests/sparse05.at

diff --git a/doc/tar.1 b/doc/tar.1
index b33f55b..d2898b6 100644
--- a/doc/tar.1
+++ b/doc/tar.1
@@ -259,6 +259,12 @@ When listing or extracting, the actual contents of \fIFILE\fR is not
 inspected, it is needed only due to syntactical requirements.  It is
 therefore common practice to use \fB/dev/null\fR in its place.
 .TP
+\fB\-\-hole\-detection\fR=\fIMETHOD\fR
+Use method to detect holes in sparse files.  This option implies
+\fB\-\-sparse\fR.  Currently there are \fIseek\fR and \fIraw\fR methods
+implemented.  Default is \fIseek\fR with fallback to \fIraw\fR when not
+applicable.
+.TP
 \fB\-G\fR, \fB\-\-incremental\fR
 Handle old GNU-format incremental backups.
 .TP
diff --git a/doc/tar.texi b/doc/tar.texi
index 9bb5a83..0e894f9 100644
--- a/doc/tar.texi
+++ b/doc/tar.texi
@@ -2748,6 +2748,13 @@ they refer to, instead of creating usual hard link members.
 @command{tar} will print out a short message summarizing the operations and
 options to @command{tar} and exit. @xref{help}.
 
+@opsummary{hole-detection}
+@item --hole-detection=@var{method}
+Use method to detect holes in sparse files.  This option implies
+@option{--sparse}.  Currently there are @var{seek} and @var{raw} methods
+implemented.  Default is @var{seek} with fallback to @var{raw} when not
+applicable. @xref{sparse}.
+
 @opsummary{ignore-case}
 @item -

Re: [Bug-tar] seek_hole proposal

2014-03-05 Thread Joerg Schilling
Pavel Raiskup  wrote:

> Hello all,
>
> I am trying to prepare patch which would reuse lseek's
> SEEK_HOLE/SEEK_DATA, the [v1] is attached.  Some info:
>
> Note that after discussion [1] I still think that existing ST_IS_SPARSE
> macro is better for file-sparseness detection than using SEEK_HOLE (not
> worth having additional syscalls open~>seek~>close).

Your code is not compatible to the SEEK_HOLE interface. A file is sparse in 
case that pathconf()/fpathconf(f, _PC_MIN_HOLE_SIZE) return a positive number 
and lseek(f, (off_t)0, SEEK_HOLE) returns a number < stat.st_size.

see "man pathconf":

 11.  If a filesystem supports  the  reporting  of  holes
  (see  lseek(2), pathconf() and fpathconf() return a
  positive number that represents  the  minimum  hole
  size  returned  in  bytes.  The  offsets  of  holes
  returned will be aligned to this same value. A spe-
  cial  value of 1 is returned if the filesystem does
  not specify the minimum hole size but still reports
  holes.

In other cases, the file stil may be sparse, but the filesystem does not 
support SEEK_HOLE. I e.g. doubt that Linux correctly implements SEEK_HOLE
for NFS.

Jörg

-- 
 EMail:jo...@schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
   j...@cs.tu-berlin.de(uni)  
   joerg.schill...@fokus.fraunhofer.de (work) Blog: 
http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/private/ ftp://ftp.berlios.de/pub/schily



Re: [Bug-tar] adding ACLs when there are none

2014-03-05 Thread Pavel Raiskup
Hello Linda,

On Tuesday, March 04, 2014 15:38:22 Linda A. Walsh wrote:
> Pavel Raiskup wrote:
> 
> > When --acls option is on (regardless of tarball contents or
> > tarball format), we should explicitly set OR delete default ACLs
> > for extracted directories.  Prior to this update, we always
> > created arbitrary default ACLs based standard file permissions.
> > 
> 
> Why would tar create any acls if there are none in the source tar?

I was not clear in the sentence probably, please read that like:
Set (if these default ACLs are also in archive) OR delete them (because
these may already be inherited from parent directory default ACLs).

--

When you pass --acls, you want to have extracted files exactly as stored
in the archive (from the ACLs perspective).  If I read correctly, in ACLs
compatbile world — no-ACLs means that you know that you don't want ACLs to
be used.

> I saw someone else have a similar complaint about acls being created
> when the tar didn't have acls but the --acls option was used.

Isn't that exactly the reason for 7fe7adcbb9 commit?  Prior to that
commit, *default* ACLs were created.  From `man acl`:

  OBJECT CREATION AND DEFAULT ACLs
   The access ACL of a file object is initialized when the object is
   created with any of the creat(), mkdir(), mknod(), mkfifo(), or
   open() functions.  If a default ACL is associated with a directory,
   the mode parameter to the functions creating file objects and the
   default ACL of the directory are used to determine the ACL of the
   new object:

> I wouldn't want a non-acl containing tarball to overwrite or change
> default acls in a directory that already exists.
>
> If I said --acl=reset or similar, that might be a desirable feature.

These are IMO candidates for omitting --acls option, no?

Or could you give an example?  What *exactly* do you expect the --acls
should behave by default?  Combine existing acls in parent directory
(default acls) with the stored in archive?

Thanks, Pavel