On 01/24/2018 03:42 AM, David Sterba wrote:
On Sun, Jan 07, 2018 at 01:54:21PM -0800, Rosen Penev wrote:
As btrfs is specific to Linux, %m can be used instead of strerror(errno)
in format strings. This has some size reduction benefits for embedded
systems.
Makes sense.
glibc, musl, and uclibc-ng all support %m as a modifier to printf.
A quick glance at the BIONIC libc source indicates that it has
support for %m as well. BSDs and Windows do not but I do believe
them to be beyond the scope of btrfs-progs.
Thanks for checking the compatibility. The %m can be substituted
by a wrapper if this becomes a problem in the future.
It seems a little problem in output now since it's not caused by this
patch.
$ touch /tmp/tmp
$ btrfs-image -r /tmp/tmp /tmp/test.img
ERROR: unable to read cluster: Success
ERROR: restore failed: Success
$ echo $?
1
Though btrfs-image failed, %m(strerror(errno) argument makes output
looks strange.
IMO, here we should judge errno before output. But it means size
reduction is inavaiable for embedded systems.
Thanks,
Su
Compiled sizes on Ubuntu 16.04:
Before:
3916512 btrfs
After:
3908744 btrfs
the delta is about 7KiB, that's not much but still counts. I would not
object further optimizations towards size reduction as long as the code
remains maintainable.
233256 libbtrfs.so.0.1
4899 bcp
2366560 btrfs-convert
2207432 btrfs-corrupt-block
13302 btrfs-debugfs
2151104 btrfs-debug-tree
2134968 btrfs-find-root
2281864 btrfs-image
2143536 btrfs-map-logical
2129704 btrfs-select-super
2151552 btrfstune
2130696 btrfs-zero-log
2276272 mkfs.btrfs
Some of the utilities are typically installed by default, the binaries
share a lot of code as they get built from the same object files. I had
once an idea of a compound binary that would switch the function by the
name of the executable. Similar to what busybox does.
https://github.com/kdave/btrfs-progs/commit/8fc697a7f763f39f3afe0abaa68ac13a49ac8a86
---
* btrfs
* mkfs.btrfs
* btrfstun
* btrfs-image
* btrfs-convert
* btrfs-debug-tree
* btrfs-show-super
* btrfs-find-root
The static target is also supported. The name of resulting boxed
binaries is btrfs.box and btrfs.box.static .
text data bss dec hex filename
550988 19120 15444 585552 8ef50 btrfs
1562099 25316 42256 1629671 18dde7 btrfs.static
659504 21104 16492 697100 aa30c btrfs.box
1817274 27988 44088 1889350 1cd446 btrfs.box.static
---
I was not sure if this is was just another good idea waiting for a usecase (or
not), so I haven't continued past the prototype. Please let me know if you'd be
interested in this functionality, the patch is fairly trivial to update.
@@ -815,7 +815,7 @@ static const char * const cmd_filesystem_sync_usage[] = {
static int cmd_filesystem_sync(int argc, char **argv)
{
- int fd, res, e;
+ int fd, res;
char *path;
DIR *dirstream = NULL;
@@ -831,10 +831,9 @@ static int cmd_filesystem_sync(int argc, char **argv)
return 1;
res = ioctl(fd, BTRFS_IOC_SYNC);
- e = errno;
close_file_or_dir(fd, dirstream);
if( res < 0 ){
- error("sync ioctl failed on '%s': %s", path, strerror(e));
+ error("sync ioctl failed on '%s': %m", path);
Let me use that one as example, there are a few more similar updates.
There's potentially lost errno from the ioctl if something inside
close_file_or_dir() overwrites it, as there are closedir and close. This
is highly unlikely and I'll deal with that separately, so I'm going to
apply the patch without further changes. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
