| Message ID | 20171214104136.GJ27580@vctlabs.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | archive handler: add preserve-attributes option | expand |
I should have given credit to Brenden Simon for an initial starting point [1], though I switched to the simpler scheme of preserve all or none instead of controlling each type of file attribute separately (separate control seems unneccesary options clutter) [1] https://github.com/sbabic/swupdate/pull/12
On 14/12/2017 11:50, sjl@vctlabs.com wrote: > I should have given credit to Brenden Simon for an initial starting point [1], though I switched to the simpler scheme of preserve all or none instead of controlling each type of file attribute separately (separate control seems unneccesary options clutter) > > [1] https://github.com/sbabic/swupdate/pull/12 > Your patch differs significatly from Brendan's. Anyway, you could add the credit in the commit message, like reworking from original idea at https://github.com/sbabic/swupdate/pull/12. Best regards, Stefano Babic
Hi, On 14/12/2017 11:41, S. Lockwood-Childs wrote: > Previously extracted files did not keep attributes stored in > the archive file, e.g. all files and dirs come out root:root > ownership and with current timestamps. > > The new preserve-attributes option asks libarchive to preserve > timestamps, uid/gid, file perm bits, acls, file attributes > (immutable, etc), and extended attributes. > > /* sw-description snippet */ > files: ( > { > filename = "some.tar.gz"; > path = "/opt"; > preserve-attributes = true; > } > ); > Where is your Signed-off-by ? You should add: Signed-off-by: S. Lockwood-Childs <sjl@dent.vctlabs.com> > Note that uid/gid preserves numeric values in a tarball, i.e. same as > tar with --numeric-owner flag. At this time libarchive doesn't support > keeping names instead. Your patch is fine. However, take a look at previous thread: http://patchwork.ozlabs.org/patch/844011/ My issue is with ARCHIVE_EXTRACT_OWNER. In fact. owner is uid of the one creating the archive - a user on the host PC. This is the user running bitbake in the Yocto's environment. And generally, this does not match with the user we want on the device, and we extract files with a uid that is not present on the target. If we really want this, I propose to add an explicitely set for it, like "preserve-owner". > --- > doc/source/sw-description.rst | 9 +++++++++ > handlers/archive_handler.c | 11 +++++++++-- > include/swupdate.h | 1 + > parser/parser.c | 1 + > 4 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst > index 4a798ee..71bdeff 100644 > --- a/doc/source/sw-description.rst > +++ b/doc/source/sw-description.rst > @@ -742,6 +742,7 @@ attribute. For example: > type = "archive"; > path = "/tmp/test"; > hook = "set_version"; > + preserve-attributes = true; > } > ); > > @@ -847,6 +848,14 @@ There are 4 main sections inside sw-description: > | | | | "filesystem" type. (path is always | > | | | | relative to the mount point.) | > +-------------+----------+------------+---------------------------------------+ > + | preserve- | bool | files | flag to control whether the following | > + | attributes | | | attributes will be preserved when | > + | | | | files are unpacked from an archive | > + | | | | (assuming destination filesystem | > + | | | | supports them, of course): | > + | | | | timestamp, uid/gid (numeric), perms, | > + | | | | file attributes, extended attributes | > + +-------------+----------+------------+---------------------------------------+ > | type | string | images | string identifier for the handler, | > | | | files | as it is set by the handler when it | > | | | scripts | regitsters itself. | > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c > index 0568a41..b4895f0 100644 > --- a/handlers/archive_handler.c > +++ b/handlers/archive_handler.c > @@ -200,11 +200,18 @@ static int install_archive_image(struct img_type *img, > return -EFAULT; > } > > - TRACE("Installing file %s on %s\n", > - img->fname, path); > + TRACE("Installing file %s on %s, %s attributes\n", > + img->fname, path, > + img->preserve_attributes ? "preserving" : "ignoring"); > > tf.flags = 0; > > + if (img->preserve_attributes) { > + tf.flags |= ARCHIVE_EXTRACT_OWNER | ARCHIVE_EXTRACT_PERM | > + ARCHIVE_EXTRACT_TIME | ARCHIVE_EXTRACT_ACL | > + ARCHIVE_EXTRACT_FFLAGS | ARCHIVE_EXTRACT_XATTR; > + } > + > ret = pthread_create(&extract_thread, &attr, extract, &tf); > if (ret) { > ERROR("Code from pthread_create() is %d\n", > diff --git a/include/swupdate.h b/include/swupdate.h > index 09e81d5..6424fc7 100644 > --- a/include/swupdate.h > +++ b/include/swupdate.h > @@ -73,6 +73,7 @@ struct img_type { > int required; > int provided; > int compressed; > + int preserve_attributes; /* whether to preserve attributes in archives */ > int is_encrypted; > int install_directly; > int is_script; > index c8a51e2..1adce54 100644 > --- a/parser/parser.c > +++ b/parser/parser.c > @@ -517,6 +517,7 @@ static int parse_files(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lua_ > strcpy(file->type, "rawfile"); > } > get_field(p, elem, "compressed", &file->compressed); > + get_field(p, elem, "preserve-attributes", &file->preserve_attributes); > get_field(p, elem, "installed-directly", &file->install_directly); > get_field(p, elem, "install-if-different", &file->id.install_if_different); > get_field(p, elem, "encrypted", &file->is_encrypted); > Best regards, Stefano Babic
On Thu, Dec 14, 2017 at 12:42:39PM +0100, Stefano Babic wrote: > Where is your Signed-off-by ? You should add: > > Signed-off-by: S. Lockwood-Childs <sjl@dent.vctlabs.com> Yes sorry I forgot '-s' when committing locally to export the patch > > > > Note that uid/gid preserves numeric values in a tarball, i.e. same as > > tar with --numeric-owner flag. At this time libarchive doesn't support > > keeping names instead. > > Your patch is fine. However, take a look at previous thread: > > http://patchwork.ozlabs.org/patch/844011/ > > My issue is with ARCHIVE_EXTRACT_OWNER. In fact. owner is uid of the one > creating the archive - a user on the host PC. This is the user running > bitbake in the Yocto's environment. And generally, this does not match > with the user we want on the device, and we extract files with a uid > that is not present on the target. > > If we really want this, I propose to add an explicitely set for it, like > "preserve-owner". It was obvious to me that old behavior of not setting owner needed to be available still, hence controlled by flag as you said in that other thread. However in my experience for embedded work uids, your observation about ownership "generally does not match" does not hold. For my projects it has been the opposite; I see fakeroot or pseudo used by build systems and SDKs when constructing tarballs with intended attributes. Using numeric uids instead of names is a bit inconvenient, but even with that constraint my team has been using useradd-staticids.bbclass so ownership still works. So, what do you want for next iteration of patch (where I fix the comment)? Do you still want 'preserve-attributes' to be split up, and if so what would the option for controlling all attributes but ownership be called (for perms, acls, and xattr)?
Hi S., On 14/12/2017 21:44, S. Lockwood-Childs wrote: > On Thu, Dec 14, 2017 at 12:42:39PM +0100, Stefano Babic wrote: >> Where is your Signed-off-by ? You should add: >> >> Signed-off-by: S. Lockwood-Childs <sjl@dent.vctlabs.com> > > Yes sorry I forgot '-s' when committing locally to export the patch > >> >> >>> Note that uid/gid preserves numeric values in a tarball, i.e. same as >>> tar with --numeric-owner flag. At this time libarchive doesn't support >>> keeping names instead. >> >> Your patch is fine. However, take a look at previous thread: >> >> http://patchwork.ozlabs.org/patch/844011/ >> >> My issue is with ARCHIVE_EXTRACT_OWNER. In fact. owner is uid of the one >> creating the archive - a user on the host PC. This is the user running >> bitbake in the Yocto's environment. And generally, this does not match >> with the user we want on the device, and we extract files with a uid >> that is not present on the target. >> >> If we really want this, I propose to add an explicitely set for it, like >> "preserve-owner". > > It was obvious to me that old behavior of not setting owner needed to > be available still, hence controlled by flag as you said in that other > thread. > > However in my experience for embedded work uids, your observation about > ownership "generally does not match" does not hold. For my projects it > has been the opposite; I see fakeroot or pseudo used by build systems and SDKs > when constructing tarballs with intended attributes. So you split your build process, and application is just built outside Yocto. > Using numeric uids instead > of names is a bit inconvenient, but even with that constraint my team has been > using useradd-staticids.bbclass so ownership still works. > > So, what do you want for next iteration of patch (where I fix the comment)? > Do you still want 'preserve-attributes' to be split up, and if so what would > the option for controlling all attributes but ownership be called (for perms, > acls, and xattr)? Mmhhh..no, this is quite messy. I do not think we need such a fine granularity. Well, I would go on with a very pragmatic way: we introduce 'preserve-attribute', including the owner, as in your patch. We could split up adding granularity in future when it will be required. Best regards, Stefano Babic
diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst index 4a798ee..71bdeff 100644 --- a/doc/source/sw-description.rst +++ b/doc/source/sw-description.rst @@ -742,6 +742,7 @@ attribute. For example: type = "archive"; path = "/tmp/test"; hook = "set_version"; + preserve-attributes = true; } ); @@ -847,6 +848,14 @@ There are 4 main sections inside sw-description: | | | | "filesystem" type. (path is always | | | | | relative to the mount point.) | +-------------+----------+------------+---------------------------------------+ + | preserve- | bool | files | flag to control whether the following | + | attributes | | | attributes will be preserved when | + | | | | files are unpacked from an archive | + | | | | (assuming destination filesystem | + | | | | supports them, of course): | + | | | | timestamp, uid/gid (numeric), perms, | + | | | | file attributes, extended attributes | + +-------------+----------+------------+---------------------------------------+ | type | string | images | string identifier for the handler, | | | | files | as it is set by the handler when it | | | | scripts | regitsters itself. | diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c index 0568a41..b4895f0 100644 --- a/handlers/archive_handler.c +++ b/handlers/archive_handler.c @@ -200,11 +200,18 @@ static int install_archive_image(struct img_type *img, return -EFAULT; } - TRACE("Installing file %s on %s\n", - img->fname, path); + TRACE("Installing file %s on %s, %s attributes\n", + img->fname, path, + img->preserve_attributes ? "preserving" : "ignoring"); tf.flags = 0; + if (img->preserve_attributes) { + tf.flags |= ARCHIVE_EXTRACT_OWNER | ARCHIVE_EXTRACT_PERM | + ARCHIVE_EXTRACT_TIME | ARCHIVE_EXTRACT_ACL | + ARCHIVE_EXTRACT_FFLAGS | ARCHIVE_EXTRACT_XATTR; + } + ret = pthread_create(&extract_thread, &attr, extract, &tf); if (ret) { ERROR("Code from pthread_create() is %d\n", diff --git a/include/swupdate.h b/include/swupdate.h index 09e81d5..6424fc7 100644 --- a/include/swupdate.h +++ b/include/swupdate.h @@ -73,6 +73,7 @@ struct img_type { int required; int provided; int compressed; + int preserve_attributes; /* whether to preserve attributes in archives */ int is_encrypted; int install_directly; int is_script; index c8a51e2..1adce54 100644 --- a/parser/parser.c +++ b/parser/parser.c @@ -517,6 +517,7 @@ static int parse_files(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lua_ strcpy(file->type, "rawfile"); } get_field(p, elem, "compressed", &file->compressed); + get_field(p, elem, "preserve-attributes", &file->preserve_attributes); get_field(p, elem, "installed-directly", &file->install_directly); get_field(p, elem, "install-if-different", &file->id.install_if_different); get_field(p, elem, "encrypted", &file->is_encrypted);
Previously extracted files did not keep attributes stored in the archive file, e.g. all files and dirs come out root:root ownership and with current timestamps. The new preserve-attributes option asks libarchive to preserve timestamps, uid/gid, file perm bits, acls, file attributes (immutable, etc), and extended attributes. /* sw-description snippet */ files: ( { filename = "some.tar.gz"; path = "/opt"; preserve-attributes = true; } ); Note that uid/gid preserves numeric values in a tarball, i.e. same as tar with --numeric-owner flag. At this time libarchive doesn't support keeping names instead. --- doc/source/sw-description.rst | 9 +++++++++ handlers/archive_handler.c | 11 +++++++++-- include/swupdate.h | 1 + parser/parser.c | 1 + 4 files changed, 20 insertions(+), 2 deletions(-)