| Message ID | 20180517192700.23457-3-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, May 17, 2018 at 12:26:57PM -0700, Luis R. Rodriguez wrote: > Both struct sb_feat_args and struct mkfs_default_params will be shared > between CLI processing and the configuration file processing added later, > so move these to their own header. The struct cli_params are CLI specific > so move them to its own CLI header as well. I'd separate out the CLI details when the CLI parsing is split into it's own file - it's not necssary to do that here. > > This will help ensure we split things neatly later and also will help > ensure the configuration file processing code from the CLI code are kept > separate and cannot touch each other's data structures. This also makes > it clear what is actually shared between both. > > There are no introduced functional changes in this commit and no > documentation changes, this is just code shuffling. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > mkfs/xfs_mkfs.c | 115 +------------------------------------------------ > mkfs/xfs_mkfs_cli.h | 65 ++++++++++++++++++++++++++++ > mkfs/xfs_mkfs_common.h | 64 +++++++++++++++++++++++++++ File names. xfs_mkfs.c is named that way by an old convention - it's the same as xfs_repair.c, xfs_db.c, etc. Every other file in the directory does not need a "xfs_mkfs_" prefix. I'd also prefer we don't have "common" as a dumping ground header. The CLI, config files, etc are all part of config processing, so config.h would be more appropriate here. Or perhaps "input.h" because they are both processing external inputs.... > 3 files changed, 131 insertions(+), 113 deletions(-) > create mode 100644 mkfs/xfs_mkfs_cli.h > create mode 100644 mkfs/xfs_mkfs_common.h > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 95cd6ced13f0..ac97039abc34 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -20,6 +20,8 @@ > #include <ctype.h> > #include "xfs_multidisk.h" > #include "libxcmd.h" > +#include "xfs_mkfs_common.h" > +#include "xfs_mkfs_cli.h" #include "config.h" > diff --git a/mkfs/xfs_mkfs_cli.h b/mkfs/xfs_mkfs_cli.h > new file mode 100644 > index 000000000000..2050814aec02 > --- /dev/null > +++ b/mkfs/xfs_mkfs_cli.h > @@ -0,0 +1,65 @@ > +#ifndef _XFS_MKFS_CLI_H > +#define _XFS_MKFS_CLI_H Missing license and copyright. It'll be the same license as the main xfs_mkfs.c file. Copyright will be Red Hat, Inc (because it's new code I wrote as dchinner@redhat.com) and you'll need to pull the date from the commit message. (Yeah, I know it's weird putting someone elses copyright on code you're moving about, but that's how it goes :P) > + > +#include "xfs_mkfs_common.h" No includes in header files if we can avoid them. [...] > diff --git a/mkfs/xfs_mkfs_common.h b/mkfs/xfs_mkfs_common.h > new file mode 100644 > index 000000000000..9b0f67b70cf1 > --- /dev/null > +++ b/mkfs/xfs_mkfs_common.h > @@ -0,0 +1,64 @@ > +#ifndef _XFS_MKFS_COMMON_H > +#define _XFS_MKFS_COMMON_H > + > +#include "libxfs.h" > + > +#include <ctype.h> Same as above for license, copyright and includes, except the copyright will also need to include the SGI copyright line from xfs_mkfs.c... Cheers, Dave.
On Fri, May 18, 2018 at 08:40:08AM +1000, Dave Chinner wrote: > On Thu, May 17, 2018 at 12:26:57PM -0700, Luis R. Rodriguez wrote: > > Both struct sb_feat_args and struct mkfs_default_params will be shared > > between CLI processing and the configuration file processing added later, > > so move these to their own header. The struct cli_params are CLI specific > > so move them to its own CLI header as well. > > I'd separate out the CLI details when the CLI parsing is split into > it's own file - it's not necssary to do that here. Alright. > > This will help ensure we split things neatly later and also will help > > ensure the configuration file processing code from the CLI code are kept > > separate and cannot touch each other's data structures. This also makes > > it clear what is actually shared between both. > > > > There are no introduced functional changes in this commit and no > > documentation changes, this is just code shuffling. > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > --- > > mkfs/xfs_mkfs.c | 115 +------------------------------------------------ > > mkfs/xfs_mkfs_cli.h | 65 ++++++++++++++++++++++++++++ > > mkfs/xfs_mkfs_common.h | 64 +++++++++++++++++++++++++++ > > File names. > > xfs_mkfs.c is named that way by an old convention - it's the same as > xfs_repair.c, xfs_db.c, etc. Every other file in the directory does > not need a "xfs_mkfs_" prefix. Yay. > I'd also prefer we don't have "common" as a dumping ground header. input.h it is. > The CLI, config files, etc are all part of config processing, so > config.h would be more appropriate here. Or perhaps "input.h" > because they are both processing external inputs.... Alrighty. > > 3 files changed, 131 insertions(+), 113 deletions(-) > > create mode 100644 mkfs/xfs_mkfs_cli.h > > create mode 100644 mkfs/xfs_mkfs_common.h > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > index 95cd6ced13f0..ac97039abc34 100644 > > --- a/mkfs/xfs_mkfs.c > > +++ b/mkfs/xfs_mkfs.c > > @@ -20,6 +20,8 @@ > > #include <ctype.h> > > #include "xfs_multidisk.h" > > #include "libxcmd.h" > > +#include "xfs_mkfs_common.h" > > +#include "xfs_mkfs_cli.h" > > #include "config.h" You mean input.h ? The config.h would be for the configuration file, no? > > diff --git a/mkfs/xfs_mkfs_cli.h b/mkfs/xfs_mkfs_cli.h > > new file mode 100644 > > index 000000000000..2050814aec02 > > --- /dev/null > > +++ b/mkfs/xfs_mkfs_cli.h > > @@ -0,0 +1,65 @@ > > +#ifndef _XFS_MKFS_CLI_H > > +#define _XFS_MKFS_CLI_H > > Missing license and copyright. It'll be the same license as the main > xfs_mkfs.c file. Copyright will be Red Hat, Inc (because it's new > code I wrote as dchinner@redhat.com) and you'll need to pull the > date from the commit message. > > (Yeah, I know it's weird putting someone elses copyright on code > you're moving about, but that's how it goes :P) I'll ignore cli.h for now. The comment below applies to the future input.h (currently the xfs_mkfs_common.h) Both data structures on input.h (sb features and the defaults) were introduced in 2017 so using that. Is the following header OK? /* * Copyright (c) 2017 Red Hat, Inc. * All Rights Reserved. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License as * published by the Free Software Foundation. * * This program is distributed in the hope that it would 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 the Free Software Foundation, * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ Took it as template from a random existing header file. I'd prefer if we just went with this as the last paragraph: * You should have received a copy of the GNU General Public License * along with this program; if not, see <http://www.gnu.org/licenses/>. It used in the kernel as well, and checkpatch now asks you to consider this. Lemme know your preference. > > + > > +#include "xfs_mkfs_common.h" > > No includes in header files if we can avoid them. OK! This cli.h file will not be added in my next iteration though. I'll avoid any header file not needed though. > [...] > > > diff --git a/mkfs/xfs_mkfs_common.h b/mkfs/xfs_mkfs_common.h > > new file mode 100644 > > index 000000000000..9b0f67b70cf1 > > --- /dev/null > > +++ b/mkfs/xfs_mkfs_common.h > > @@ -0,0 +1,64 @@ > > +#ifndef _XFS_MKFS_COMMON_H > > +#define _XFS_MKFS_COMMON_H > > + > > +#include "libxfs.h" > > + > > +#include <ctype.h> > > Same as above for license, copyright and includes, except the > copyright will also need to include the SGI copyright line from > xfs_mkfs.c... Why SGI? I don't see anything from SGI on input.h. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 17, 2018 at 04:54:03PM -0700, Luis R. Rodriguez wrote: > On Fri, May 18, 2018 at 08:40:08AM +1000, Dave Chinner wrote: > > On Thu, May 17, 2018 at 12:26:57PM -0700, Luis R. Rodriguez wrote: > > > Both struct sb_feat_args and struct mkfs_default_params will be shared > > > between CLI processing and the configuration file processing added later, > > > so move these to their own header. The struct cli_params are CLI specific > > > so move them to its own CLI header as well. > > > > I'd separate out the CLI details when the CLI parsing is split into > > it's own file - it's not necssary to do that here. > > Alright. > > > > This will help ensure we split things neatly later and also will help > > > ensure the configuration file processing code from the CLI code are kept > > > separate and cannot touch each other's data structures. This also makes > > > it clear what is actually shared between both. > > > > > > There are no introduced functional changes in this commit and no > > > documentation changes, this is just code shuffling. > > > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > > --- > > > mkfs/xfs_mkfs.c | 115 +------------------------------------------------ > > > mkfs/xfs_mkfs_cli.h | 65 ++++++++++++++++++++++++++++ > > > mkfs/xfs_mkfs_common.h | 64 +++++++++++++++++++++++++++ > > > > File names. > > > > xfs_mkfs.c is named that way by an old convention - it's the same as > > xfs_repair.c, xfs_db.c, etc. Every other file in the directory does > > not need a "xfs_mkfs_" prefix. > > Yay. > > > I'd also prefer we don't have "common" as a dumping ground header. > > input.h it is. > > > The CLI, config files, etc are all part of config processing, so > > config.h would be more appropriate here. Or perhaps "input.h" > > because they are both processing external inputs.... > > Alrighty. > > > > 3 files changed, 131 insertions(+), 113 deletions(-) > > > create mode 100644 mkfs/xfs_mkfs_cli.h > > > create mode 100644 mkfs/xfs_mkfs_common.h > > > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > > index 95cd6ced13f0..ac97039abc34 100644 > > > --- a/mkfs/xfs_mkfs.c > > > +++ b/mkfs/xfs_mkfs.c > > > @@ -20,6 +20,8 @@ > > > #include <ctype.h> > > > #include "xfs_multidisk.h" > > > #include "libxcmd.h" > > > +#include "xfs_mkfs_common.h" > > > +#include "xfs_mkfs_cli.h" > > > > #include "config.h" > > You mean input.h ? The config.h would be for the configuration file, no? I suggested both config.h and input.h as potential candidates. Given that you use config.h in a later patch, i'd suggest that config.h is the right name for this. > Both data structures on input.h (sb features and the defaults) were > introduced in 2017 so using that. Is the following header OK? > > /* > * Copyright (c) 2017 Red Hat, Inc. > * All Rights Reserved. > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License as > * published by the Free Software Foundation. > * > * This program is distributed in the hope that it would 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 the Free Software Foundation, > * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > */ > > Took it as template from a random existing header file. > > I'd prefer if we just went with this as the last paragraph: > > * You should have received a copy of the GNU General Public License > * along with this program; if not, see <http://www.gnu.org/licenses/>. > > It used in the kernel as well, and checkpatch now asks you to consider > this. > > Lemme know your preference. Use the same as existing files. If we're going to update the licensing info, then we need to do it for everything, move to spdx identifiers in the code and use a single copy of each license in the LICENSE file. That's for another day, though.... > > > @@ -0,0 +1,64 @@ > > > +#ifndef _XFS_MKFS_COMMON_H > > > +#define _XFS_MKFS_COMMON_H > > > + > > > +#include "libxfs.h" > > > + > > > +#include <ctype.h> > > > > Same as above for license, copyright and includes, except the > > copyright will also need to include the SGI copyright line from > > xfs_mkfs.c... > > Why SGI? I don't see anything from SGI on input.h. Because the sb_feats structure is derived from much older code abstractions - it pre-existed all refactoring work I did.... Cheers, Dave.
On Fri, May 18, 2018 at 10:49:58AM +1000, Dave Chinner wrote: > On Thu, May 17, 2018 at 04:54:03PM -0700, Luis R. Rodriguez wrote: > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > > > index 95cd6ced13f0..ac97039abc34 100644 > > > > --- a/mkfs/xfs_mkfs.c > > > > +++ b/mkfs/xfs_mkfs.c > > > > @@ -20,6 +20,8 @@ > > > > #include <ctype.h> > > > > #include "xfs_multidisk.h" > > > > #include "libxcmd.h" > > > > +#include "xfs_mkfs_common.h" > > > > +#include "xfs_mkfs_cli.h" > > > > > > #include "config.h" > > > > You mean input.h ? The config.h would be for the configuration file, no? > > I suggested both config.h and input.h as potential candidates. Given > that you use config.h in a later patch, i'd suggest that config.h is > the right name for this. And the configuration header name? Luis -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 95cd6ced13f0..ac97039abc34 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -20,6 +20,8 @@ #include <ctype.h> #include "xfs_multidisk.h" #include "libxcmd.h" +#include "xfs_mkfs_common.h" +#include "xfs_mkfs_cli.h" @@ -706,98 +708,6 @@ cli_opt_set( opts->subopt_params[subopt].str_seen; } -/* - * Shared superblock configuration options - * - * These options provide shared configuration tunables for the filesystem - * superblock. There are three possible sources for these options set, each - * source can overriding the later source: - * - * o built-in defaults - * o configuration file (XXX) - * o command line - * - * These values are not used directly - they are inputs into the mkfs geometry - * validation. - */ -struct sb_feat_args { - int log_version; - int attr_version; - int dir_version; - bool inode_align; /* XFS_SB_VERSION_ALIGNBIT */ - bool nci; /* XFS_SB_VERSION_BORGBIT */ - bool lazy_sb_counters; /* XFS_SB_VERSION2_LAZYSBCOUNTBIT */ - bool parent_pointers; /* XFS_SB_VERSION2_PARENTBIT */ - bool projid32bit; /* XFS_SB_VERSION2_PROJID32BIT */ - bool crcs_enabled; /* XFS_SB_VERSION2_CRCBIT */ - bool dirftype; /* XFS_SB_VERSION2_FTYPE */ - bool finobt; /* XFS_SB_FEAT_RO_COMPAT_FINOBT */ - bool spinodes; /* XFS_SB_FEAT_INCOMPAT_SPINODES */ - bool rmapbt; /* XFS_SB_FEAT_RO_COMPAT_RMAPBT */ - bool reflink; /* XFS_SB_FEAT_RO_COMPAT_REFLINK */ - bool nodalign; - bool nortalign; -}; - -/* - * Options configured on the command line. - * - * This stores all the specific config parameters the user sets on the command - * line. We don't keep flags to indicate what parameters are set - if we need - * to check if an option was set on the command line, we check the relevant - * entry in the option table which records whether it was specified in the - * .seen and .str_seen variables in the table. - * - * Some parameters are stored as strings for post-parsing after their dependent - * options have been resolved (e.g. block size and sector size have been parsed - * and validated). - * - * This allows us to check that values have been set without needing separate - * flags for each value, and hence avoids needing to record and check for each - * specific option that can set the value later on in the code. In the cases - * where we don't have a cli_params structure around, the function cli_opt_set() - * function can be used. - */ -struct cli_params { - int sectorsize; - int blocksize; - - /* parameters that depend on sector/block size being validated. */ - char *dsize; - char *agsize; - char *dsu; - char *dirblocksize; - char *logsize; - char *lsu; - char *rtextsize; - char *rtsize; - - /* parameters where 0 is a valid CLI value */ - int dsunit; - int dswidth; - int dsw; - int64_t logagno; - int loginternal; - int lsunit; - - /* parameters where 0 is not a valid value */ - int64_t agcount; - int inodesize; - int inopblock; - int imaxpct; - int lsectorsize; - uuid_t uuid; - - /* feature flags that are set */ - struct sb_feat_args sb_feat; - - /* root inode characteristics */ - struct fsxattr fsx; - - /* libxfs device setup */ - struct libxfs_xinit *xi; -}; - /* * Calculated filesystem feature and geometry information. * @@ -850,27 +760,6 @@ struct mkfs_params { struct sb_feat_args sb_feat; }; -/* - * Default filesystem features and configuration values - * - * This structure contains the default mkfs values that are to be used when - * a user does not specify the option on the command line. We do not use these - * values directly - they are inputs to the mkfs geometry validation and - * calculations. - */ -struct mkfs_default_params { - char *source; /* where the defaults came from */ - - int sectorsize; - int blocksize; - - /* feature flags that are set */ - struct sb_feat_args sb_feat; - - /* root inode characteristics */ - struct fsxattr fsx; -}; - static void __attribute__((noreturn)) usage( void ) { diff --git a/mkfs/xfs_mkfs_cli.h b/mkfs/xfs_mkfs_cli.h new file mode 100644 index 000000000000..2050814aec02 --- /dev/null +++ b/mkfs/xfs_mkfs_cli.h @@ -0,0 +1,65 @@ +#ifndef _XFS_MKFS_CLI_H +#define _XFS_MKFS_CLI_H + +#include "xfs_mkfs_common.h" + +/* + * Options configured on the command line. + * + * This stores all the specific config parameters the user sets on the command + * line. We don't keep flags to indicate what parameters are set - if we need + * to check if an option was set on the command line, we check the relevant + * entry in the option table which records whether it was specified in the + * .seen and .str_seen variables in the table. + * + * Some parameters are stored as strings for post-parsing after their dependent + * options have been resolved (e.g. block size and sector size have been parsed + * and validated). + * + * This allows us to check that values have been set without needing separate + * flags for each value, and hence avoids needing to record and check for each + * specific option that can set the value later on in the code. In the cases + * where we don't have a cli_params structure around, the function cli_opt_set() + * function can be used. + */ +struct cli_params { + int sectorsize; + int blocksize; + + /* parameters that depend on sector/block size being validated. */ + char *dsize; + char *agsize; + char *dsu; + char *dirblocksize; + char *logsize; + char *lsu; + char *rtextsize; + char *rtsize; + + /* parameters where 0 is a valid CLI value */ + int dsunit; + int dswidth; + int dsw; + int64_t logagno; + int loginternal; + int lsunit; + + /* parameters where 0 is not a valid value */ + int64_t agcount; + int inodesize; + int inopblock; + int imaxpct; + int lsectorsize; + uuid_t uuid; + + /* feature flags that are set */ + struct sb_feat_args sb_feat; + + /* root inode characteristics */ + struct fsxattr fsx; + + /* libxfs device setup */ + struct libxfs_xinit *xi; +}; + +#endif /* _XFS_MKFS_CLI_H */ diff --git a/mkfs/xfs_mkfs_common.h b/mkfs/xfs_mkfs_common.h new file mode 100644 index 000000000000..9b0f67b70cf1 --- /dev/null +++ b/mkfs/xfs_mkfs_common.h @@ -0,0 +1,64 @@ +#ifndef _XFS_MKFS_COMMON_H +#define _XFS_MKFS_COMMON_H + +#include "libxfs.h" + +#include <ctype.h> + +struct fsxattr; + +/* + * Shared superblock configuration options + * + * These options provide shared configuration tunables for the filesystem + * superblock. There are three possible sources for these options set, each + * source can overriding the later source: + * + * o built-in defaults + * o configuration file (XXX) + * o command line + * + * These values are not used directly - they are inputs into the mkfs geometry + * validation. + */ +struct sb_feat_args { + int log_version; + int attr_version; + int dir_version; + bool inode_align; /* XFS_SB_VERSION_ALIGNBIT */ + bool nci; /* XFS_SB_VERSION_BORGBIT */ + bool lazy_sb_counters; /* XFS_SB_VERSION2_LAZYSBCOUNTBIT */ + bool parent_pointers; /* XFS_SB_VERSION2_PARENTBIT */ + bool projid32bit; /* XFS_SB_VERSION2_PROJID32BIT */ + bool crcs_enabled; /* XFS_SB_VERSION2_CRCBIT */ + bool dirftype; /* XFS_SB_VERSION2_FTYPE */ + bool finobt; /* XFS_SB_FEAT_RO_COMPAT_FINOBT */ + bool spinodes; /* XFS_SB_FEAT_INCOMPAT_SPINODES */ + bool rmapbt; /* XFS_SB_FEAT_RO_COMPAT_RMAPBT */ + bool reflink; /* XFS_SB_FEAT_RO_COMPAT_REFLINK */ + bool nodalign; + bool nortalign; +}; + +/* + * Default filesystem features and configuration values + * + * This structure contains the default mkfs values that are to be used when + * a user does not specify the option on the command line. We do not use these + * values directly - they are inputs to the mkfs geometry validation and + * calculations. + */ +struct mkfs_default_params { + char *source; /* where the defaults came from */ + + int sectorsize; + int blocksize; + + /* feature flags that are set */ + struct sb_feat_args sb_feat; + + /* root inode characteristics */ + struct fsxattr fsx; +}; + +#endif /* _XFS_MKFS_COMMON_H */
Both struct sb_feat_args and struct mkfs_default_params will be shared between CLI processing and the configuration file processing added later, so move these to their own header. The struct cli_params are CLI specific so move them to its own CLI header as well. This will help ensure we split things neatly later and also will help ensure the configuration file processing code from the CLI code are kept separate and cannot touch each other's data structures. This also makes it clear what is actually shared between both. There are no introduced functional changes in this commit and no documentation changes, this is just code shuffling. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- mkfs/xfs_mkfs.c | 115 +------------------------------------------------ mkfs/xfs_mkfs_cli.h | 65 ++++++++++++++++++++++++++++ mkfs/xfs_mkfs_common.h | 64 +++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 113 deletions(-) create mode 100644 mkfs/xfs_mkfs_cli.h create mode 100644 mkfs/xfs_mkfs_common.h