| Message ID | 1358928771-31960-2-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1/23/13 2:12 AM, Anand Jain wrote: > The definition of the function open_file_or_dir() is moved from common.c > to utils.c in order to be able to share some common code between scrub > and the device stats in the following step. That common code uses > open_file_or_dir(). Since open_file_or_dir() makes use of the function > dirfd(3), the required XOPEN version was raised from 6 to 7. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > Original-Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de> Cool, I had this on my stack too. But can you maybe remove the nonsensical return values, and instead of renaming & keeping the btrfsctl.c copy, why not just use a common copy in utils.c? It'd just be 2 checks for fd < 0 in the btrfsctl callers. Thanks, -Eric > --- > Makefile | 4 ++-- > btrfsctl.c | 7 ++++--- > cmds-balance.c | 1 + > cmds-inspect.c | 1 + > cmds-qgroup.c | 1 + > cmds-quota.c | 1 + > cmds-subvolume.c | 1 + > commands.h | 3 --- > common.c | 46 ---------------------------------------------- > utils.c | 30 ++++++++++++++++++++++++++++-- > utils.h | 3 +++ > 11 files changed, 42 insertions(+), 56 deletions(-) > delete mode 100644 common.c > > diff --git a/Makefile b/Makefile > index 4894903..8576d90 100644 > --- a/Makefile > +++ b/Makefile > @@ -41,8 +41,8 @@ all: version $(progs) manpages > version: > bash version.sh > > -btrfs: $(objects) btrfs.o help.o common.o $(cmds_objects) > - $(CC) $(CFLAGS) -o btrfs btrfs.o help.o common.o $(cmds_objects) \ > +btrfs: $(objects) btrfs.o help.o $(cmds_objects) > + $(CC) $(CFLAGS) -o btrfs btrfs.o help.o $(cmds_objects) \ > $(objects) $(LDFLAGS) $(LIBS) -lpthread > > calc-size: $(objects) calc-size.o > diff --git a/btrfsctl.c b/btrfsctl.c > index 518684c..049a5f3 100644 > --- a/btrfsctl.c > +++ b/btrfsctl.c > @@ -63,7 +63,7 @@ static void print_usage(void) > exit(1); > } > > -static int open_file_or_dir(const char *fname) > +static int btrfsctl_open_file_or_dir(const char *fname) > { > int ret; > struct stat st; > @@ -91,6 +91,7 @@ static int open_file_or_dir(const char *fname) > } > return fd; > } > + > int main(int ac, char **av) > { > char *fname = NULL; > @@ -128,7 +129,7 @@ int main(int ac, char **av) > snap_location = strdup(fullpath); > snap_location = dirname(snap_location); > > - snap_fd = open_file_or_dir(snap_location); > + snap_fd = btrfsctl_open_file_or_dir(snap_location); > > name = strdup(fullpath); > name = basename(name); > @@ -238,7 +239,7 @@ int main(int ac, char **av) > } > name = fname; > } else { > - fd = open_file_or_dir(fname); > + fd = btrfsctl_open_file_or_dir(fname); > } > > if (name) { > diff --git a/cmds-balance.c b/cmds-balance.c > index 38a7426..6268b61 100644 > --- a/cmds-balance.c > +++ b/cmds-balance.c > @@ -28,6 +28,7 @@ > #include "volumes.h" > > #include "commands.h" > +#include "utils.h" > > static const char * const balance_cmd_group_usage[] = { > "btrfs [filesystem] balance <command> [options] <path>", > diff --git a/cmds-inspect.c b/cmds-inspect.c > index edabff5..79e069b 100644 > --- a/cmds-inspect.c > +++ b/cmds-inspect.c > @@ -22,6 +22,7 @@ > > #include "kerncompat.h" > #include "ioctl.h" > +#include "utils.h" > > #include "commands.h" > #include "btrfs-list.h" > diff --git a/cmds-qgroup.c b/cmds-qgroup.c > index 1525c11..cafc284 100644 > --- a/cmds-qgroup.c > +++ b/cmds-qgroup.c > @@ -24,6 +24,7 @@ > #include "ioctl.h" > > #include "commands.h" > +#include "utils.h" > > static const char * const qgroup_cmd_group_usage[] = { > "btrfs qgroup <command> [options] <path>", > diff --git a/cmds-quota.c b/cmds-quota.c > index cf9ad97..8481514 100644 > --- a/cmds-quota.c > +++ b/cmds-quota.c > @@ -23,6 +23,7 @@ > #include "ioctl.h" > > #include "commands.h" > +#include "utils.h" > > static const char * const quota_cmd_group_usage[] = { > "btrfs quota <command> [options] <path>", > diff --git a/cmds-subvolume.c b/cmds-subvolume.c > index ac39f7b..e3cdb1e 100644 > --- a/cmds-subvolume.c > +++ b/cmds-subvolume.c > @@ -32,6 +32,7 @@ > #include "ctree.h" > #include "commands.h" > #include "btrfs-list.h" > +#include "utils.h" > > static const char * const subvolume_cmd_group_usage[] = { > "btrfs subvolume <command> <args>", > diff --git a/commands.h b/commands.h > index bb6d2dd..8114a73 100644 > --- a/commands.h > +++ b/commands.h > @@ -79,9 +79,6 @@ void help_ambiguous_token(const char *arg, const struct cmd_group *grp); > > void help_command_group(const struct cmd_group *grp, int argc, char **argv); > > -/* common.c */ > -int open_file_or_dir(const char *fname); > - > extern const struct cmd_group subvolume_cmd_group; > extern const struct cmd_group filesystem_cmd_group; > extern const struct cmd_group balance_cmd_group; > diff --git a/common.c b/common.c > deleted file mode 100644 > index 03f6570..0000000 > --- a/common.c > +++ /dev/null > @@ -1,46 +0,0 @@ > -/* > - * This program is free software; you can redistribute it and/or > - * modify it under the terms of the GNU General Public > - * License v2 as published by the Free Software Foundation. > - * > - * This program is distributed in the hope that it will be useful, > - * but WITHOUT ANY WARRANTY; without even the implied warranty of > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > - * General Public License for more details. > - * > - * You should have received a copy of the GNU General Public > - * License along with this program; if not, write to the > - * Free Software Foundation, Inc., 59 Temple Place - Suite 330, > - * Boston, MA 021110-1307, USA. > - */ > - > -#include <sys/types.h> > -#include <sys/stat.h> > -#include <dirent.h> > -#include <fcntl.h> > - > -int open_file_or_dir(const char *fname) > -{ > - int ret; > - struct stat st; > - DIR *dirstream; > - int fd; > - > - ret = stat(fname, &st); > - if (ret < 0) { > - return -1; > - } > - if (S_ISDIR(st.st_mode)) { > - dirstream = opendir(fname); > - if (!dirstream) { > - return -2; > - } > - fd = dirfd(dirstream); > - } else { > - fd = open(fname, O_RDWR); > - } > - if (fd < 0) { > - return -3; > - } > - return fd; > -} > diff --git a/utils.c b/utils.c > index 205e667..774f81d 100644 > --- a/utils.c > +++ b/utils.c > @@ -16,8 +16,9 @@ > * Boston, MA 021110-1307, USA. > */ > > -#define _XOPEN_SOURCE 600 > -#define __USE_XOPEN2K > +#define _XOPEN_SOURCE 700 > +#define __USE_XOPEN2K8 > +#define __XOPEN2K8 /* due to an error in dirent.h, to get dirfd() */ > #include <stdio.h> > #include <stdlib.h> > #ifndef __CHECKER__ > @@ -1220,3 +1221,28 @@ scan_again: > return 0; > } > > +int open_file_or_dir(const char *fname) > +{ > + int ret; > + struct stat st; > + DIR *dirstream; > + int fd; > + > + ret = stat(fname, &st); > + if (ret < 0) { > + return -1; > + } > + if (S_ISDIR(st.st_mode)) { > + dirstream = opendir(fname); > + if (!dirstream) { > + return -2; > + } > + fd = dirfd(dirstream); > + } else { > + fd = open(fname, O_RDWR); > + } > + if (fd < 0) { > + return -3; > + } > + return fd; > +} > diff --git a/utils.h b/utils.h > index 3a0368b..6975f10 100644 > --- a/utils.h > +++ b/utils.h > @@ -19,6 +19,8 @@ > #ifndef __UTILS__ > #define __UTILS__ > > +#include "ctree.h" > + > #define BTRFS_MKFS_SYSTEM_GROUP_SIZE (4 * 1024 * 1024) > > int make_btrfs(int fd, const char *device, const char *label, > @@ -46,4 +48,5 @@ int check_label(char *input); > int get_mountpt(char *dev, char *mntpt, size_t size); > > int btrfs_scan_block_devices(int run_ioctl); > +int open_file_or_dir(const char *fname); > #endif > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 23 Jan 2013 22:39:29 -0600, Eric Sandeen wrote:
> instead of renaming & keeping the btrfsctl.c copy
There is a new momentum to improve the Btrfs-progs quality :)
IMO, one step is to get rid of the legacy tools and sources. It wastes
time to maintain them and these old tools cause confusion. btrfsctl.c,
btrfs-vol.c and btrfs-show.c are not needed anymore. Please correct me
if there are plans to use these old tools in future Linux distributions.
The "btrfs" tool replaces the legacy "btrfsctl", "btrfs-vol" and
"btrfs-show" tools. Below, the usage text of the old tools is quoted.
All these tasks are also offered in the "btrfs" tool, and this tool is
the newer one.
Some time ago I sent a patch to remove these legacy tools from the
default build
<http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg16919.html>.
We should also remove the sources. Any objections against this?
If we don't have btrfsctl.c anymore, we don't need to worry about
open_file_or_dir() in that file. We could either leave Anand's patch as
it is and afterwards remove btrfsctl.c, or remove the file first and
then the open_file_or_dir() patch can be shrunken.
usage: btrfsctl [ -d file|dir] [ -s snap_name subvol|tree ]
[-r size] [-A device] [-a] [-c] [-D dir .]
-d filename: defragments one file
-d directory: defragments the entire Btree
-s snap_name dir: creates a new snapshot of dir
-S subvol_name dir: creates a new subvolume
-r [+-]size[gkm]: resize the FS by size amount
-A device: scans the device file for a Btrfs filesystem
-a: scans all devices for Btrfs filesystems
-c: forces a single FS sync
-D: delete snapshot
-m [tree id] directory: set the default mounted subvolume to the
[tree id] or the directory
usage: btrfs-vol [options] mount_point
-a device add one device
-b balance chunks across all devices
-r device remove one device
usage: btrfs-show [search label or device]
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/24/2013 10:23 AM, Stefan Behrens wrote: > On Wed, 23 Jan 2013 22:39:29 -0600, Eric Sandeen wrote: >> instead of renaming& keeping the btrfsctl.c copy > > There is a new momentum to improve the Btrfs-progs quality :) > > IMO, one step is to get rid of the legacy tools and sources. It wastes > time to maintain them and these old tools cause confusion. btrfsctl.c, > btrfs-vol.c and btrfs-show.c are not needed anymore. Please correct me > if there are plans to use these old tools in future Linux distributions. > The "btrfs" tool replaces the legacy "btrfsctl", "btrfs-vol" and > "btrfs-show" tools. Below, the usage text of the old tools is quoted. > All these tasks are also offered in the "btrfs" tool, and this tool is > the newer one. I fully agree: btrfsctl, btrfs-vol, btrfs-show are perfectly replaced by by btrfs. Moreover time to time the patches are more complex than the needing because exists these "legacy" programs. I checked the debian package, and to me seems that there is no need of {btrfsctl,btrfs-vol,btrfs-show} BR Goffredo
On 1/24/13 11:57 AM, Goffredo Baroncelli wrote: > On 01/24/2013 10:23 AM, Stefan Behrens wrote: >> On Wed, 23 Jan 2013 22:39:29 -0600, Eric Sandeen wrote: >>> instead of renaming& keeping the btrfsctl.c copy >> >> There is a new momentum to improve the Btrfs-progs quality :) >> >> IMO, one step is to get rid of the legacy tools and sources. It wastes >> time to maintain them and these old tools cause confusion. btrfsctl.c, >> btrfs-vol.c and btrfs-show.c are not needed anymore. Please correct me >> if there are plans to use these old tools in future Linux distributions. >> The "btrfs" tool replaces the legacy "btrfsctl", "btrfs-vol" and >> "btrfs-show" tools. Below, the usage text of the old tools is quoted. >> All these tasks are also offered in the "btrfs" tool, and this tool is >> the newer one. > > I fully agree: btrfsctl, btrfs-vol, btrfs-show are perfectly replaced by by btrfs. Moreover time to time the patches are more complex than the needing because exists these "legacy" programs. > > I checked the debian package, and to me seems that there is no need of {btrfsctl,btrfs-vol,btrfs-show} Hm, they are shipped in the Fedora package. For backwards compat, could those be turned into shell scripts which invoke the btrfs tool? -Eric > BR > Goffredo > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/24/2013 08:42 PM, Eric Sandeen wrote: > On 1/24/13 11:57 AM, Goffredo Baroncelli wrote: >> On 01/24/2013 10:23 AM, Stefan Behrens wrote: >>> On Wed, 23 Jan 2013 22:39:29 -0600, Eric Sandeen wrote: >>>> instead of renaming& keeping the btrfsctl.c copy >>> >>> There is a new momentum to improve the Btrfs-progs quality :) >>> >>> IMO, one step is to get rid of the legacy tools and sources. It >>> wastes time to maintain them and these old tools cause confusion. >>> btrfsctl.c, btrfs-vol.c and btrfs-show.c are not needed anymore. >>> Please correct me if there are plans to use these old tools in >>> future Linux distributions. The "btrfs" tool replaces the legacy >>> "btrfsctl", "btrfs-vol" and "btrfs-show" tools. Below, the usage >>> text of the old tools is quoted. All these tasks are also offered >>> in the "btrfs" tool, and this tool is the newer one. >> >> I fully agree: btrfsctl, btrfs-vol, btrfs-show are perfectly >> replaced by by btrfs. Moreover time to time the patches are more >> complex than the needing because exists these "legacy" programs. >> >> I checked the debian package, and to me seems that there is no need >> of {btrfsctl,btrfs-vol,btrfs-show} > > Hm, they are shipped in the Fedora package. The same is true for the debian package, but are these used in Fedora ? > > For backwards compat, could those be turned into shell scripts which > invoke the btrfs tool? I don't see any gain to maintains a script bash (which has to be written from scratch) instead of maintains the current C code. These programs were deprecated two years ago [1]. If some distribution need them, could maintain them as separate patch. But I think that the mainstream should remove. > > -Eric > >> BR Goffredo >> > > [1] $ git log 002d021c^..002d021c commit 002d021c5f2d838394e850e304546ffad283518a Author: Goffredo Baroncelli <kreijack@libero.it> Date: Sun Dec 5 17:47:36 2010 +0000 Deprecate btrfsctl, btrfs-show, btrfs-vol Hi all, the patch below deprecates the following programs * btrfsctl * btrfs-vol * btrfs-show the reason is simple, these programs are superseded by the btrfs utility, both in terms of documentation, usability and bug. The goal is to avoid to duplicate codes and avoid update two programs. The patch adds a warning in the man pages, in the INSTALL file and in the programs. [...]
On Thu, Jan 24, 2013 at 03:09:53PM -0700, Goffredo Baroncelli wrote: > On 01/24/2013 08:42 PM, Eric Sandeen wrote: > > On 1/24/13 11:57 AM, Goffredo Baroncelli wrote: > >> On 01/24/2013 10:23 AM, Stefan Behrens wrote: > >>> On Wed, 23 Jan 2013 22:39:29 -0600, Eric Sandeen wrote: > >>>> instead of renaming& keeping the btrfsctl.c copy > >>> > >>> There is a new momentum to improve the Btrfs-progs quality :) > >>> > >>> IMO, one step is to get rid of the legacy tools and sources. It > >>> wastes time to maintain them and these old tools cause confusion. > >>> btrfsctl.c, btrfs-vol.c and btrfs-show.c are not needed anymore. > >>> Please correct me if there are plans to use these old tools in > >>> future Linux distributions. The "btrfs" tool replaces the legacy > >>> "btrfsctl", "btrfs-vol" and "btrfs-show" tools. Below, the usage > >>> text of the old tools is quoted. All these tasks are also offered > >>> in the "btrfs" tool, and this tool is the newer one. > >> > >> I fully agree: btrfsctl, btrfs-vol, btrfs-show are perfectly > >> replaced by by btrfs. Moreover time to time the patches are more > >> complex than the needing because exists these "legacy" programs. > >> > >> I checked the debian package, and to me seems that there is no need > >> of {btrfsctl,btrfs-vol,btrfs-show} > > > > Hm, they are shipped in the Fedora package. > > The same is true for the debian package, but are these used in Fedora ? > > > > > For backwards compat, could those be turned into shell scripts which > > invoke the btrfs tool? > > I don't see any gain to maintains a script bash (which has to be written > from scratch) instead of maintains the current C code. > > These programs were deprecated two years ago [1]. If some distribution > need them, could maintain them as separate patch. But I think that the > mainstream should remove. I'd say that if SuSE or oracle depend on them we keep them. Otherwise, I'm fine with removing them or just making the 3 line bash script. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 24, 2013 at 05:36:52PM -0500, Chris Mason wrote: > On Thu, Jan 24, 2013 at 03:09:53PM -0700, Goffredo Baroncelli wrote: > > The same is true for the debian package, but are these used in Fedora ? > > > For backwards compat, could those be turned into shell scripts which > > > invoke the btrfs tool? > > > > I don't see any gain to maintains a script bash (which has to be written > > from scratch) instead of maintains the current C code. > > > > These programs were deprecated two years ago [1]. If some distribution > > need them, could maintain them as separate patch. But I think that the > > mainstream should remove. > > I'd say that if SuSE or oracle depend on them we keep them. Otherwise, > I'm fine with removing them or just making the 3 line bash script. I wanted to remove them once from our packages, but some tool uses them. I'm fine with replacing them with a shellscript, this is just a syntactic conversion from btrfsctl style to the subcommands. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Chris, On 25/01/2013, at 9:36 AM, Chris Mason <chris.mason@fusionio.com> wrote: > I'd say that if SuSE or oracle depend on them we keep them. Otherwise, > I'm fine with removing them or just making the 3 line bash script. You can take this as an official response from Oracle: we don't need/want the old tools. :) All of our documentation uses the unified binary. Thanks, Avi -- Oracle <http://www.oracle.com> Avi Miller | Principal Program Manager | +61 (412) 229 687 Oracle Linux and Virtualization 417 St Kilda Road, Melbourne, Victoria 3004 Australia -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Cool, I had this on my stack too. But can you maybe remove the > nonsensical return values, and instead of renaming & keeping the btrfsctl.c > copy, why not just use a common copy in utils.c? It'd just be 2 checks > for fd < 0 in the btrfsctl callers. Thanks for the comments Eric. Though I agree, but it deviates from the purpose of this patch-set. It could be taken as a separate patch. Anand -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/24/13 1:42 PM, Eric Sandeen wrote: > On 1/24/13 11:57 AM, Goffredo Baroncelli wrote: >> On 01/24/2013 10:23 AM, Stefan Behrens wrote: >>> On Wed, 23 Jan 2013 22:39:29 -0600, Eric Sandeen wrote: >>>> instead of renaming& keeping the btrfsctl.c copy >>> >>> There is a new momentum to improve the Btrfs-progs quality :) >>> >>> IMO, one step is to get rid of the legacy tools and sources. It wastes >>> time to maintain them and these old tools cause confusion. btrfsctl.c, >>> btrfs-vol.c and btrfs-show.c are not needed anymore. Please correct me >>> if there are plans to use these old tools in future Linux distributions. >>> The "btrfs" tool replaces the legacy "btrfsctl", "btrfs-vol" and >>> "btrfs-show" tools. Below, the usage text of the old tools is quoted. >>> All these tasks are also offered in the "btrfs" tool, and this tool is >>> the newer one. >> >> I fully agree: btrfsctl, btrfs-vol, btrfs-show are perfectly replaced by by btrfs. Moreover time to time the patches are more complex than the needing because exists these "legacy" programs. >> >> I checked the debian package, and to me seems that there is no need of {btrfsctl,btrfs-vol,btrfs-show} > > Hm, they are shipped in the Fedora package. > > For backwards compat, could those be turned into shell scripts which invoke the btrfs tool? Turns out anaconda is using btrfsctl for resizing: class BTRFS(FS): ... _resizefs = "btrfsctl" ... @property def resizeArgs(self): argv = ["-r", "%dm" % (self.targetSize,), self.device] return argv but that should be trivial to replace w/ btrfs filesystem resize [devid:][+/-]<newsize>[gkm]|[devid:]max <path> I'll ping the anaconda guys, don't let this use stop you :) -Eric > -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/24/13 4:09 PM, Goffredo Baroncelli wrote: > On 01/24/2013 08:42 PM, Eric Sandeen wrote: >> On 1/24/13 11:57 AM, Goffredo Baroncelli wrote: >>> On 01/24/2013 10:23 AM, Stefan Behrens wrote: >>>> On Wed, 23 Jan 2013 22:39:29 -0600, Eric Sandeen wrote: >>>>> instead of renaming& keeping the btrfsctl.c copy >>>> >>>> There is a new momentum to improve the Btrfs-progs quality :) >>>> >>>> IMO, one step is to get rid of the legacy tools and sources. It >>>> wastes time to maintain them and these old tools cause confusion. >>>> btrfsctl.c, btrfs-vol.c and btrfs-show.c are not needed anymore. >>>> Please correct me if there are plans to use these old tools in >>>> future Linux distributions. The "btrfs" tool replaces the legacy >>>> "btrfsctl", "btrfs-vol" and "btrfs-show" tools. Below, the usage >>>> text of the old tools is quoted. All these tasks are also offered >>>> in the "btrfs" tool, and this tool is the newer one. >>> >>> I fully agree: btrfsctl, btrfs-vol, btrfs-show are perfectly >>> replaced by by btrfs. Moreover time to time the patches are more >>> complex than the needing because exists these "legacy" programs. >>> >>> I checked the debian package, and to me seems that there is no need >>> of {btrfsctl,btrfs-vol,btrfs-show} >> >> Hm, they are shipped in the Fedora package. > > The same is true for the debian package, but are these used in Fedora ? > >> >> For backwards compat, could those be turned into shell scripts which >> invoke the btrfs tool? > > I don't see any gain to maintains a script bash (which has to be > written from scratch) instead of maintains the current C code. It should be a trivial bash script to convert the calls, and it should require very little maintenance. Much less than the hundreds of lines of duplicated C code, I think. If nobody needs them, though, no reason for even a bash script. David, Suse may be using them now, but probably can adapt? Anaconda said it could drop the use of btrfsctl. :) -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 25, 2013 at 10:14:06AM -0600, Eric Sandeen wrote: > On 1/24/13 4:09 PM, Goffredo Baroncelli wrote: > > On 01/24/2013 08:42 PM, Eric Sandeen wrote: > >> On 1/24/13 11:57 AM, Goffredo Baroncelli wrote: > >>> On 01/24/2013 10:23 AM, Stefan Behrens wrote: > >>>> On Wed, 23 Jan 2013 22:39:29 -0600, Eric Sandeen wrote: > >>>>> instead of renaming& keeping the btrfsctl.c copy > >>>> > >>>> There is a new momentum to improve the Btrfs-progs quality :) > >>>> > >>>> IMO, one step is to get rid of the legacy tools and sources. It > >>>> wastes time to maintain them and these old tools cause confusion. > >>>> btrfsctl.c, btrfs-vol.c and btrfs-show.c are not needed anymore. > >>>> Please correct me if there are plans to use these old tools in > >>>> future Linux distributions. The "btrfs" tool replaces the legacy > >>>> "btrfsctl", "btrfs-vol" and "btrfs-show" tools. Below, the usage > >>>> text of the old tools is quoted. All these tasks are also offered > >>>> in the "btrfs" tool, and this tool is the newer one. > >>> > >>> I fully agree: btrfsctl, btrfs-vol, btrfs-show are perfectly > >>> replaced by by btrfs. Moreover time to time the patches are more > >>> complex than the needing because exists these "legacy" programs. > >>> > >>> I checked the debian package, and to me seems that there is no need > >>> of {btrfsctl,btrfs-vol,btrfs-show} > >> > >> Hm, they are shipped in the Fedora package. > > > > The same is true for the debian package, but are these used in Fedora ? > > > >> > >> For backwards compat, could those be turned into shell scripts which > >> invoke the btrfs tool? > > > > I don't see any gain to maintains a script bash (which has to be > > written from scratch) instead of maintains the current C code. > > It should be a trivial bash script to convert the calls, and it should > require very little maintenance. Much less than the hundreds of lines > of duplicated C code, I think. > > If nobody needs them, though, no reason for even a bash script. > > David, Suse may be using them now, but probably can adapt? > Anaconda said it could drop the use of btrfsctl. :) I've just asked someone I know at Canonical, and he says there's no use of these tools in the Ubuntu installer. (Disclaimer: it's not entirely his area, and there's probably other places to look, like udev rules, but on a cursory glance, it should be OK). I've also checked with the Debian installer people, and they're not using the deprecated tools either. Further, these searches: http://codesearch.debian.net/search?q=btrfs-show http://codesearch.debian.net/search?q=btrfs-vol http://codesearch.debian.net/search?q=btrfsctl suggest that there's very little impact over the rest of the system as well. Hugo.
On 01/25/2013 11:48 AM, Hugo Mills wrote: > On Fri, Jan 25, 2013 at 10:14:06AM -0600, Eric Sandeen wrote: >> On 1/24/13 4:09 PM, Goffredo Baroncelli wrote: >>> On 01/24/2013 08:42 PM, Eric Sandeen wrote: >>>> On 1/24/13 11:57 AM, Goffredo Baroncelli wrote: >>>>> On 01/24/2013 10:23 AM, Stefan Behrens wrote: >>>>>> On Wed, 23 Jan 2013 22:39:29 -0600, Eric Sandeen wrote: >>>>>>> instead of renaming& keeping the btrfsctl.c copy >>>>>> There is a new momentum to improve the Btrfs-progs quality :) >>>>>> >>>>>> IMO, one step is to get rid of the legacy tools and sources. It >>>>>> wastes time to maintain them and these old tools cause confusion. >>>>>> btrfsctl.c, btrfs-vol.c and btrfs-show.c are not needed anymore. >>>>>> Please correct me if there are plans to use these old tools in >>>>>> future Linux distributions. The "btrfs" tool replaces the legacy >>>>>> "btrfsctl", "btrfs-vol" and "btrfs-show" tools. Below, the usage >>>>>> text of the old tools is quoted. All these tasks are also offered >>>>>> in the "btrfs" tool, and this tool is the newer one. >>>>> I fully agree: btrfsctl, btrfs-vol, btrfs-show are perfectly >>>>> replaced by by btrfs. Moreover time to time the patches are more >>>>> complex than the needing because exists these "legacy" programs. >>>>> >>>>> I checked the debian package, and to me seems that there is no need >>>>> of {btrfsctl,btrfs-vol,btrfs-show} >>>> Hm, they are shipped in the Fedora package. >>> The same is true for the debian package, but are these used in Fedora ? >>> >>>> For backwards compat, could those be turned into shell scripts which >>>> invoke the btrfs tool? >>> I don't see any gain to maintains a script bash (which has to be >>> written from scratch) instead of maintains the current C code. >> It should be a trivial bash script to convert the calls, and it should >> require very little maintenance. Much less than the hundreds of lines >> of duplicated C code, I think. >> >> If nobody needs them, though, no reason for even a bash script. >> >> David, Suse may be using them now, but probably can adapt? >> Anaconda said it could drop the use of btrfsctl. :) > I've just asked someone I know at Canonical, and he says there's no > use of these tools in the Ubuntu installer. (Disclaimer: it's not > entirely his area, and there's probably other places to look, like > udev rules, but on a cursory glance, it should be OK). > > I've also checked with the Debian installer people, and they're not > using the deprecated tools either. Further, these searches: > > http://codesearch.debian.net/search?q=btrfs-show > http://codesearch.debian.net/search?q=btrfs-vol > http://codesearch.debian.net/search?q=btrfsctl > > suggest that there's very little impact over the rest of the system as > well. > > Hugo. > I took a look at Stefan's patch to Makefile and only the building and installation of the legacy apps has been eliminated. This is a good first step so that if someone does need one of these apps, they can still be made. Then, at some later time, remove the targets in Makefile and delete the source files. Or, do it all now? [my preference] Comments? Gene -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Good to have the old src-code removed as well. yum snapshot plugin is out of btrfsctl. http://lists.baseurl.org/pipermail/yum-devel/2012-July/009396.html Thanks, Anand -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Makefile b/Makefile index 4894903..8576d90 100644 --- a/Makefile +++ b/Makefile @@ -41,8 +41,8 @@ all: version $(progs) manpages version: bash version.sh -btrfs: $(objects) btrfs.o help.o common.o $(cmds_objects) - $(CC) $(CFLAGS) -o btrfs btrfs.o help.o common.o $(cmds_objects) \ +btrfs: $(objects) btrfs.o help.o $(cmds_objects) + $(CC) $(CFLAGS) -o btrfs btrfs.o help.o $(cmds_objects) \ $(objects) $(LDFLAGS) $(LIBS) -lpthread calc-size: $(objects) calc-size.o diff --git a/btrfsctl.c b/btrfsctl.c index 518684c..049a5f3 100644 --- a/btrfsctl.c +++ b/btrfsctl.c @@ -63,7 +63,7 @@ static void print_usage(void) exit(1); } -static int open_file_or_dir(const char *fname) +static int btrfsctl_open_file_or_dir(const char *fname) { int ret; struct stat st; @@ -91,6 +91,7 @@ static int open_file_or_dir(const char *fname) } return fd; } + int main(int ac, char **av) { char *fname = NULL; @@ -128,7 +129,7 @@ int main(int ac, char **av) snap_location = strdup(fullpath); snap_location = dirname(snap_location); - snap_fd = open_file_or_dir(snap_location); + snap_fd = btrfsctl_open_file_or_dir(snap_location); name = strdup(fullpath); name = basename(name); @@ -238,7 +239,7 @@ int main(int ac, char **av) } name = fname; } else { - fd = open_file_or_dir(fname); + fd = btrfsctl_open_file_or_dir(fname); } if (name) { diff --git a/cmds-balance.c b/cmds-balance.c index 38a7426..6268b61 100644 --- a/cmds-balance.c +++ b/cmds-balance.c @@ -28,6 +28,7 @@ #include "volumes.h" #include "commands.h" +#include "utils.h" static const char * const balance_cmd_group_usage[] = { "btrfs [filesystem] balance <command> [options] <path>", diff --git a/cmds-inspect.c b/cmds-inspect.c index edabff5..79e069b 100644 --- a/cmds-inspect.c +++ b/cmds-inspect.c @@ -22,6 +22,7 @@ #include "kerncompat.h" #include "ioctl.h" +#include "utils.h" #include "commands.h" #include "btrfs-list.h" diff --git a/cmds-qgroup.c b/cmds-qgroup.c index 1525c11..cafc284 100644 --- a/cmds-qgroup.c +++ b/cmds-qgroup.c @@ -24,6 +24,7 @@ #include "ioctl.h" #include "commands.h" +#include "utils.h" static const char * const qgroup_cmd_group_usage[] = { "btrfs qgroup <command> [options] <path>", diff --git a/cmds-quota.c b/cmds-quota.c index cf9ad97..8481514 100644 --- a/cmds-quota.c +++ b/cmds-quota.c @@ -23,6 +23,7 @@ #include "ioctl.h" #include "commands.h" +#include "utils.h" static const char * const quota_cmd_group_usage[] = { "btrfs quota <command> [options] <path>", diff --git a/cmds-subvolume.c b/cmds-subvolume.c index ac39f7b..e3cdb1e 100644 --- a/cmds-subvolume.c +++ b/cmds-subvolume.c @@ -32,6 +32,7 @@ #include "ctree.h" #include "commands.h" #include "btrfs-list.h" +#include "utils.h" static const char * const subvolume_cmd_group_usage[] = { "btrfs subvolume <command> <args>", diff --git a/commands.h b/commands.h index bb6d2dd..8114a73 100644 --- a/commands.h +++ b/commands.h @@ -79,9 +79,6 @@ void help_ambiguous_token(const char *arg, const struct cmd_group *grp); void help_command_group(const struct cmd_group *grp, int argc, char **argv); -/* common.c */ -int open_file_or_dir(const char *fname); - extern const struct cmd_group subvolume_cmd_group; extern const struct cmd_group filesystem_cmd_group; extern const struct cmd_group balance_cmd_group; diff --git a/common.c b/common.c deleted file mode 100644 index 03f6570..0000000 --- a/common.c +++ /dev/null @@ -1,46 +0,0 @@ -/* - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public - * License v2 as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * General Public License for more details. - * - * You should have received a copy of the GNU General Public - * License along with this program; if not, write to the - * Free Software Foundation, Inc., 59 Temple Place - Suite 330, - * Boston, MA 021110-1307, USA. - */ - -#include <sys/types.h> -#include <sys/stat.h> -#include <dirent.h> -#include <fcntl.h> - -int open_file_or_dir(const char *fname) -{ - int ret; - struct stat st; - DIR *dirstream; - int fd; - - ret = stat(fname, &st); - if (ret < 0) { - return -1; - } - if (S_ISDIR(st.st_mode)) { - dirstream = opendir(fname); - if (!dirstream) { - return -2; - } - fd = dirfd(dirstream); - } else { - fd = open(fname, O_RDWR); - } - if (fd < 0) { - return -3; - } - return fd; -} diff --git a/utils.c b/utils.c index 205e667..774f81d 100644 --- a/utils.c +++ b/utils.c @@ -16,8 +16,9 @@ * Boston, MA 021110-1307, USA. */ -#define _XOPEN_SOURCE 600 -#define __USE_XOPEN2K +#define _XOPEN_SOURCE 700 +#define __USE_XOPEN2K8 +#define __XOPEN2K8 /* due to an error in dirent.h, to get dirfd() */ #include <stdio.h> #include <stdlib.h> #ifndef __CHECKER__ @@ -1220,3 +1221,28 @@ scan_again: return 0; } +int open_file_or_dir(const char *fname) +{ + int ret; + struct stat st; + DIR *dirstream; + int fd; + + ret = stat(fname, &st); + if (ret < 0) { + return -1; + } + if (S_ISDIR(st.st_mode)) { + dirstream = opendir(fname); + if (!dirstream) { + return -2; + } + fd = dirfd(dirstream); + } else { + fd = open(fname, O_RDWR); + } + if (fd < 0) { + return -3; + } + return fd; +} diff --git a/utils.h b/utils.h index 3a0368b..6975f10 100644 --- a/utils.h +++ b/utils.h @@ -19,6 +19,8 @@ #ifndef __UTILS__ #define __UTILS__ +#include "ctree.h" + #define BTRFS_MKFS_SYSTEM_GROUP_SIZE (4 * 1024 * 1024) int make_btrfs(int fd, const char *device, const char *label, @@ -46,4 +48,5 @@ int check_label(char *input); int get_mountpt(char *dev, char *mntpt, size_t size); int btrfs_scan_block_devices(int run_ioctl); +int open_file_or_dir(const char *fname); #endif
The definition of the function open_file_or_dir() is moved from common.c to utils.c in order to be able to share some common code between scrub and the device stats in the following step. That common code uses open_file_or_dir(). Since open_file_or_dir() makes use of the function dirfd(3), the required XOPEN version was raised from 6 to 7. Signed-off-by: Anand Jain <anand.jain@oracle.com> Original-Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de> --- Makefile | 4 ++-- btrfsctl.c | 7 ++++--- cmds-balance.c | 1 + cmds-inspect.c | 1 + cmds-qgroup.c | 1 + cmds-quota.c | 1 + cmds-subvolume.c | 1 + commands.h | 3 --- common.c | 46 ---------------------------------------------- utils.c | 30 ++++++++++++++++++++++++++++-- utils.h | 3 +++ 11 files changed, 42 insertions(+), 56 deletions(-) delete mode 100644 common.c