| Message ID | 20170720092932.32580-7-jtulak@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Jul 20, 2017 at 11:29:31AM +0200, Jan Tulak wrote: > +/* > + * Get and set values to the opts table. > + */ > +static inline uint64_t > +get_conf_val(int opt, int subopt) > +{ > + return opts[opt].subopt_params[subopt].value; > +} > + > +static void > +set_conf_val(int opt, int subopt, uint64_t val) > +{ > + struct subopt_param *sp = &opts[opt].subopt_params[subopt]; These are pass by value so its fine to do it this way. > + > + sp->value = val; > +} > + > static inline void > set_conf_raw(int opt, int subopt, const char *value) > { > @@ -880,6 +905,18 @@ getnum( > } > > /* > + * A wrapper for getnum and set_conf_val. > + */ > +static inline uint64_t > +parse_conf_val(int opt, int subopt, char *value) > +{ > + uint64_t num = getnum(value, &opts[opt], subopt); > + > + set_conf_val(opt, subopt, num); > + return num; > +} A good comment explaining getnum() will abort if parsing for some reasons fails is worth it given the way you use parse_conf_val() is rather inconsistent -- in some places you use the return value, in some other you do not, and someone reading the code should hopefully get a sense of a relief that error checking is *not* required given it is already built-in to the helpers used by parse_conf_val(). This also limits the use of parse_conf_val() in that we won't be able to customize error messages with better context, so its a good time to evaluate if we want to continue with that tradition or let parse_conf_val() return int and if it returns 0 it was successful, otherwise an error value. Making this return an int and have the return value checked makes this code IMHO cleaner and subject to growth. Right now this sort of hack IMHO limits the code to growth, despite the fact that I know this would create more work at the moment. Thoughts? 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 27/07/2017 18:18, Luis R. Rodriguez wrote: > On Thu, Jul 20, 2017 at 11:29:31AM +0200, Jan Tulak wrote: >> +/* >> + * Get and set values to the opts table. >> + */ >> +static inline uint64_t >> +get_conf_val(int opt, int subopt) >> +{ >> + return opts[opt].subopt_params[subopt].value; >> +} >> + >> +static void >> +set_conf_val(int opt, int subopt, uint64_t val) >> +{ >> + struct subopt_param *sp = &opts[opt].subopt_params[subopt]; > These are pass by value so its fine to do it this way. > >> + >> + sp->value = val; >> +} >> + >> static inline void >> set_conf_raw(int opt, int subopt, const char *value) >> { >> @@ -880,6 +905,18 @@ getnum( >> } >> >> /* >> + * A wrapper for getnum and set_conf_val. >> + */ >> +static inline uint64_t >> +parse_conf_val(int opt, int subopt, char *value) >> +{ >> + uint64_t num = getnum(value, &opts[opt], subopt); >> + >> + set_conf_val(opt, subopt, num); >> + return num; >> +} > A good comment explaining getnum() will abort if parsing for some reasons fails > is worth it given the way you use parse_conf_val() is rather inconsistent -- in > some places you use the return value, in some other you do not, and someone > reading the code should hopefully get a sense of a relief that error checking > is *not* required given it is already built-in to the helpers used by > parse_conf_val(). Good idea. > > This also limits the use of parse_conf_val() in that we won't be able to > customize error messages with better context, so its a good time to evaluate > if we want to continue with that tradition or let parse_conf_val() return > int and if it returns 0 it was successful, otherwise an error value. Quoting also the other email (Re: [PATCH 1/5 v2] mkfs: replace variables with opts table: -b,d,s options): > Here parse_conf_val() is used only to update D_AGCOUNT based on the parsed > value. But note that the return value is ignored. In some other places the > return value is used. This is inconsistent, and I realize that the reason > we ignore errors is that getnum() is used, however now is a good time to > consider if we should just allow the caller to check the error value and > return a proper error message and bail on their own. This would allow for > better contextual error messages as the code grows. We can discuss this in > the patch that adds parse_conf_val(). And do we need better messages? If some value is out of range, or it is a conflict, there is not much context needed, and better to not have to care about these errors... Do you have an example when it would be helpful? If it just spits out a return code, you have to add a check to every use and you will have many times the same code like what is in getnum() at this moment: /* Validity check the result. */ if (c == E_SMALL) illegal_option(str, opts, index, _("value is too small")); else if (c == E_BIG) illegal_option(str, opts, index, _("value is too large")); else if (c == E_PO2) illegal_option(str, opts, index, _("value must be a power of 2")); ... and other checks for conflicts, etc... I think this goes exactly against what I'm trying to do... Jan > > Making this return an int and have the return value checked makes this code > IMHO cleaner and subject to growth. Right now this sort of hack IMHO limits > the code to growth, despite the fact that I know this would create more work > at the moment. > -- 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 Fri, Jul 28, 2017 at 04:44:50PM +0200, Jan Tulak wrote: > On 27/07/2017 18:18, Luis R. Rodriguez wrote: > > On Thu, Jul 20, 2017 at 11:29:31AM +0200, Jan Tulak wrote: > > This also limits the use of parse_conf_val() in that we won't be able to > > customize error messages with better context, so its a good time to evaluate > > if we want to continue with that tradition or let parse_conf_val() return > > int and if it returns 0 it was successful, otherwise an error value. > Quoting also the other email (Re: [PATCH 1/5 v2] mkfs: replace variables > with opts table: -b,d,s options): > > Here parse_conf_val() is used only to update D_AGCOUNT based on the parsed > > value. But note that the return value is ignored. In some other places the > > return value is used. This is inconsistent, and I realize that the reason > > we ignore errors is that getnum() is used, however now is a good time to > > consider if we should just allow the caller to check the error value and > > return a proper error message and bail on their own. This would allow for > > better contextual error messages as the code grows. We can discuss this in > > the patch that adds parse_conf_val(). > > And do we need better messages? We could later when parsing the configuration file, yes. An abort is fine too but seems rather grotesque. But also more importantly this is a matter of style and I realize the old style is to just abort/assert, so I figured it would be a good time now to ask ourselves if we want proper error messages dealt with / passed slowly moving forward. But yes, I do suspect we may want better error messages when parsing these. This may mean for instance we want a string built into the struct that defines the sobopt so we can use it to inform userspace using a pointer to the description rather than doing a switch statement on each one and interpreting back to plain english. > If some value is out of range, or it is a > conflict, there is not much context needed, and better to not have to care > about these errors... Do you have an example when it would be helpful? If it > just spits out a return code, you have to add a check to every use and you > will have many times the same code like what is in getnum() at this moment Not really, if we are parsing say D_AGCOUNT we could have a member as part of the struct, say "description" then we can use say subopt[D_AGCOUNT].description on the error message, perhaps the only thing that would change for instance would be the context on which the error was run into, command line option passed or config file read, say with the filename and line number. How would we be able to detect an error happened and pass exactly where the error happened otherwise on a config file for example? 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 29/07/2017 19:02, Luis R. Rodriguez wrote: > On Fri, Jul 28, 2017 at 04:44:50PM +0200, Jan Tulak wrote: >> And do we need better messages? > We could later when parsing the configuration file, yes. An abort is fine too but > seems rather grotesque. But also more importantly this is a matter of style and I > realize the old style is to just abort/assert, so I figured it would be a good time > now to ask ourselves if we want proper error messages dealt with / passed slowly > moving forward. > > But yes, I do suspect we may want better error messages when parsing these. This > may mean for instance we want a string built into the struct that defines the sobopt > so we can use it to inform userspace using a pointer to the description rather than > doing a switch statement on each one and interpreting back to plain english. > The reason for better messages is reasonable. About the style... well, it makes sense. But I would certainly not do this in this set. So, I think about a way how to keep the current behavior, but slowly build up the ground for something like what you suggest. Using the same style of error-returning logic from the other email ([PATCH 1/7] mkfs: Save raw ...), the error argument pointer would be optional. So, when you do parse_conf_val(OPT_D, D_AGCOUNT, str, NULL); then you get the old behavior and any error causes a termination inside of this function. But if you instead pass some pointer: parse_conf_val(OPT_D, D_AGCOUNT, str, &err); then right now, we would print the message but do not terminate. And later on, some other message handling can be added. >> If some value is out of range, or it is a >> conflict, there is not much context needed, and better to not have to care >> about these errors... Do you have an example when it would be helpful? If it >> just spits out a return code, you have to add a check to every use and you >> will have many times the same code like what is in getnum() at this moment > Not really, if we are parsing say D_AGCOUNT we could have a member as part of the > struct, say "description" then we can use say subopt[D_AGCOUNT].description on > the error message, perhaps the only thing that would change for instance would be > the context on which the error was run into, command line option passed or config > file read, say with the filename and line number. > > How would we be able to detect an error happened and pass exactly where the > error happened otherwise on a config file for example? Yes, I see the issue - getnum finds an error, but it doesn't know the line in the config file to report it. But with what I write above about the error handling, this could work. if (c < sp->minval) { if (config_file) illegal_config(str, line, opts, index, _("value is too small"), err); else illegal_option(str, opts, index, _("value is too small"), err); } ... and later in the code, if you are in the config file, you could do something like: parse_conf_val(opt, subopt, str, &err); if (err) report_invalid_line(current_line); Thoughts? Jan -- 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 Wed, Aug 02, 2017 at 04:43:09PM +0200, Jan Tulak wrote: > On 29/07/2017 19:02, Luis R. Rodriguez wrote: > > On Fri, Jul 28, 2017 at 04:44:50PM +0200, Jan Tulak wrote: > > > And do we need better messages? > > We could later when parsing the configuration file, yes. An abort is fine too but > > seems rather grotesque. But also more importantly this is a matter of style and I > > realize the old style is to just abort/assert, so I figured it would be a good time > > now to ask ourselves if we want proper error messages dealt with / passed slowly > > moving forward. > > > > But yes, I do suspect we may want better error messages when parsing these. This > > may mean for instance we want a string built into the struct that defines the sobopt > > so we can use it to inform userspace using a pointer to the description rather than > > doing a switch statement on each one and interpreting back to plain english. > > > The reason for better messages is reasonable. About the style... well, it > makes sense. But I would certainly not do this in this set. So, I think > about a way how to keep the current behavior, but slowly build up the ground > for something like what you suggest. > > Using the same style of error-returning logic from the other email ([PATCH > 1/7] mkfs: Save raw ...), the error argument pointer would be optional. So, > when you do parse_conf_val(OPT_D, D_AGCOUNT, str, NULL); then you get the > old behavior and any error causes a termination inside of this function. But > if you instead pass some pointer: parse_conf_val(OPT_D, D_AGCOUNT, str, > &err); then right now, we would print the message but do not terminate. And > later on, some other message handling can be added. I think this is grotesque. Also, how would we know an error did happen then? > > > If some value is out of range, or it is a > > > conflict, there is not much context needed, and better to not have to care > > > about these errors... Do you have an example when it would be helpful? If it > > > just spits out a return code, you have to add a check to every use and you > > > will have many times the same code like what is in getnum() at this moment > > Not really, if we are parsing say D_AGCOUNT we could have a member as part of the > > struct, say "description" then we can use say subopt[D_AGCOUNT].description on > > the error message, perhaps the only thing that would change for instance would be > > the context on which the error was run into, command line option passed or config > > file read, say with the filename and line number. > > > > How would we be able to detect an error happened and pass exactly where the > > error happened otherwise on a config file for example? > Yes, I see the issue - getnum finds an error, but it doesn't know the line > in the config file to report it. But with what I write above about the error > handling, this could work. > > if (c < sp->minval) { > if (config_file) illegal_config(str, line, opts, index, _("value is too > small"), err); How would it know what str and line are? > else illegal_option(str, opts, index, _("value is too small"), err); > } This seems convoluted and I don't really like it one bit. > ... and later in the code, if you are in the config file, you could do > something like: > > parse_conf_val(opt, subopt, str, &err); > if (err) report_invalid_line(current_line); > > Thoughts? Just my take: I prefer we do the right thing from the start. Even if it takes us ages to move forward, baby steps, and clean patches and evolutions, moving slowly away from the old crazy habits. 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 02/08/2017 18:57, Luis R. Rodriguez wrote: > On Wed, Aug 02, 2017 at 04:43:09PM +0200, Jan Tulak wrote: >> On 29/07/2017 19:02, Luis R. Rodriguez wrote: >>> On Fri, Jul 28, 2017 at 04:44:50PM +0200, Jan Tulak wrote: >>>> And do we need better messages? >>> We could later when parsing the configuration file, yes. An abort is fine too but >>> seems rather grotesque. But also more importantly this is a matter of style and I >>> realize the old style is to just abort/assert, so I figured it would be a good time >>> now to ask ourselves if we want proper error messages dealt with / passed slowly >>> moving forward. >>> >>> But yes, I do suspect we may want better error messages when parsing these. This >>> may mean for instance we want a string built into the struct that defines the sobopt >>> so we can use it to inform userspace using a pointer to the description rather than >>> doing a switch statement on each one and interpreting back to plain english. >>> >> The reason for better messages is reasonable. About the style... well, it >> makes sense. But I would certainly not do this in this set. So, I think >> about a way how to keep the current behavior, but slowly build up the ground >> for something like what you suggest. >> >> Using the same style of error-returning logic from the other email ([PATCH >> 1/7] mkfs: Save raw ...), the error argument pointer would be optional. So, >> when you do parse_conf_val(OPT_D, D_AGCOUNT, str, NULL); then you get the >> old behavior and any error causes a termination inside of this function. But >> if you instead pass some pointer: parse_conf_val(OPT_D, D_AGCOUNT, str, >> &err); then right now, we would print the message but do not terminate. And >> later on, some other message handling can be added. > I think this is grotesque. Also, how would we know an error did happen then? Just test if err is 0 or not, same as with errno.h. And the dual behavior would be only a temporary measure. Once all uses are converted to the new behavior, the terminating part can be dropped and the error argument will become mandatory. > >>>> If some value is out of range, or it is a >>>> conflict, there is not much context needed, and better to not have to care >>>> about these errors... Do you have an example when it would be helpful? If it >>>> just spits out a return code, you have to add a check to every use and you >>>> will have many times the same code like what is in getnum() at this moment >>> Not really, if we are parsing say D_AGCOUNT we could have a member as part of the >>> struct, say "description" then we can use say subopt[D_AGCOUNT].description on >>> the error message, perhaps the only thing that would change for instance would be >>> the context on which the error was run into, command line option passed or config >>> file read, say with the filename and line number. >>> >>> How would we be able to detect an error happened and pass exactly where the >>> error happened otherwise on a config file for example? >> Yes, I see the issue - getnum finds an error, but it doesn't know the line >> in the config file to report it. But with what I write above about the error >> handling, this could work. >> >> if (c < sp->minval) { >> if (config_file) illegal_config(str, line, opts, index, _("value is too >> small"), err); > How would it know what str and line are? The "line" argument is a mistake, it shouldn't be here - it is solved by the snipped below. The "str" is already there, it is what getnum() parses - only the "err" at the end would be added. > >> else illegal_option(str, opts, index, _("value is too small"), err); >> } > This seems convoluted and I don't really like it one bit. > >> ... and later in the code, if you are in the config file, you could do >> something like: >> >> parse_conf_val(opt, subopt, str, &err); >> if (err) report_invalid_line(current_line); >> >> Thoughts? > Just my take: I prefer we do the right thing from the start. Even > if it takes us ages to move forward, baby steps, and clean patches > and evolutions, moving slowly away from the old crazy habits. In which case, I would just continue as we do now (terminating on an error), and then change it in the whole mkfs at once in some other patch set. There always will be something old and ugly we have to use temporarily, or we end up stashing one patchset after another, always trying to fix some other thing first, and never really fixing anything. Jan -- 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 Wed, Aug 02, 2017 at 08:11:38PM +0200, Jan Tulak wrote: > On 02/08/2017 18:57, Luis R. Rodriguez wrote: > > On Wed, Aug 02, 2017 at 04:43:09PM +0200, Jan Tulak wrote: > > > The reason for better messages is reasonable. About the style... well, it > > > makes sense. But I would certainly not do this in this set. So, I think > > > about a way how to keep the current behavior, but slowly build up the ground > > > for something like what you suggest. > > > > > > Using the same style of error-returning logic from the other email ([PATCH > > > 1/7] mkfs: Save raw ...), the error argument pointer would be optional. So, > > > when you do parse_conf_val(OPT_D, D_AGCOUNT, str, NULL); then you get the > > > old behavior and any error causes a termination inside of this function. But > > > if you instead pass some pointer: parse_conf_val(OPT_D, D_AGCOUNT, str, > > > &err); then right now, we would print the message but do not terminate. And > > > later on, some other message handling can be added. > > I think this is grotesque. Also, how would we know an error did happen then? > > Just test if err is 0 or not, same as with errno.h. And the dual behavior > would be only a temporary measure. Once all uses are converted to the new > behavior, the terminating part can be dropped and the error argument will > become mandatory. Yes, I think this is not nice if I understood what you say below correctly. > > > > > If some value is out of range, or it is a > > > > > conflict, there is not much context needed, and better to not have to care > > > > > about these errors... Do you have an example when it would be helpful? If it > > > > > just spits out a return code, you have to add a check to every use and you > > > > > will have many times the same code like what is in getnum() at this moment > > > > Not really, if we are parsing say D_AGCOUNT we could have a member as part of the > > > > struct, say "description" then we can use say subopt[D_AGCOUNT].description on > > > > the error message, perhaps the only thing that would change for instance would be > > > > the context on which the error was run into, command line option passed or config > > > > file read, say with the filename and line number. > > > > > > > > How would we be able to detect an error happened and pass exactly where the > > > > error happened otherwise on a config file for example? > > > Yes, I see the issue - getnum finds an error, but it doesn't know the line > > > in the config file to report it. But with what I write above about the error > > > handling, this could work. > > > > > > if (c < sp->minval) { > > > if (config_file) illegal_config(str, line, opts, index, _("value is too > > > small"), err); > > How would it know what str and line are? > > The "line" argument is a mistake, it shouldn't be here - it is solved by the > snipped below. The "str" is already there, it is what getnum() parses - only > the "err" at the end would be added. So you mean the error code would only be visible on the print out, but not passed to the caller as a starting step. A secondary step would go and change that to then return the error code and have each caller check it? > > > > > else illegal_option(str, opts, index, _("value is too small"), err); > > > } > > This seems convoluted and I don't really like it one bit. > > > > > ... and later in the code, if you are in the config file, you could do > > > something like: > > > > > > parse_conf_val(opt, subopt, str, &err); > > > if (err) report_invalid_line(current_line); > > > > > > Thoughts? > > Just my take: I prefer we do the right thing from the start. Even > > if it takes us ages to move forward, baby steps, and clean patches > > and evolutions, moving slowly away from the old crazy habits. > > In which case, I would just continue as we do now (terminating on an error), > and then change it in the whole mkfs at once in some other patch set. There > always will be something old and ugly we have to use temporarily, or we end > up stashing one patchset after another, always trying to fix some other > thing first, and never really fixing anything. It seems like a change which could be fit into your series, it is more work, but then again most of the work here was expected to be significant and doing away with a lot of crap. Up to you, I just am providing my own feedback and making clear the requirements for the config stuff. I think it will be silly to add config support without having the ability to catch errors and then indicate on its own however it chooses exactly where the error occurred. Will you take on doing the error code changes after ? 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 02/08/2017 21:48, Luis R. Rodriguez wrote: > On Wed, Aug 02, 2017 at 08:11:38PM +0200, Jan Tulak wrote: >> On 02/08/2017 18:57, Luis R. Rodriguez wrote: >>> On Wed, Aug 02, 2017 at 04:43:09PM +0200, Jan Tulak wrote: >>>> The reason for better messages is reasonable. About the style... well, it >>>> makes sense. But I would certainly not do this in this set. So, I think >>>> about a way how to keep the current behavior, but slowly build up the ground >>>> for something like what you suggest. >>>> >>>> Using the same style of error-returning logic from the other email ([PATCH >>>> 1/7] mkfs: Save raw ...), the error argument pointer would be optional. So, >>>> when you do parse_conf_val(OPT_D, D_AGCOUNT, str, NULL); then you get the >>>> old behavior and any error causes a termination inside of this function. But >>>> if you instead pass some pointer: parse_conf_val(OPT_D, D_AGCOUNT, str, >>>> &err); then right now, we would print the message but do not terminate. And >>>> later on, some other message handling can be added. >>> I think this is grotesque. Also, how would we know an error did happen then? >> Just test if err is 0 or not, same as with errno.h. And the dual behavior >> would be only a temporary measure. Once all uses are converted to the new >> behavior, the terminating part can be dropped and the error argument will >> become mandatory. > Yes, I think this is not nice if I understood what you say below correctly. > >>>>>> If some value is out of range, or it is a >>>>>> conflict, there is not much context needed, and better to not have to care >>>>>> about these errors... Do you have an example when it would be helpful? If it >>>>>> just spits out a return code, you have to add a check to every use and you >>>>>> will have many times the same code like what is in getnum() at this moment >>>>> Not really, if we are parsing say D_AGCOUNT we could have a member as part of the >>>>> struct, say "description" then we can use say subopt[D_AGCOUNT].description on >>>>> the error message, perhaps the only thing that would change for instance would be >>>>> the context on which the error was run into, command line option passed or config >>>>> file read, say with the filename and line number. >>>>> >>>>> How would we be able to detect an error happened and pass exactly where the >>>>> error happened otherwise on a config file for example? >>>> Yes, I see the issue - getnum finds an error, but it doesn't know the line >>>> in the config file to report it. But with what I write above about the error >>>> handling, this could work. >>>> >>>> if (c < sp->minval) { >>>> if (config_file) illegal_config(str, line, opts, index, _("value is too >>>> small"), err); >>> How would it know what str and line are? >> The "line" argument is a mistake, it shouldn't be here - it is solved by the >> snipped below. The "str" is already there, it is what getnum() parses - only >> the "err" at the end would be added. > So you mean the error code would only be visible on the print out, but not > passed to the caller as a starting step. A secondary step would go and change > that to then return the error code and have each caller check it? Actually, it would go both ways. Passing it down to the printout may not be necessary, depending on what you want to do with it in the config case, but it would be send up as well. > >>>> else illegal_option(str, opts, index, _("value is too small"), err); >>>> } >>> This seems convoluted and I don't really like it one bit. >>> >>>> ... and later in the code, if you are in the config file, you could do >>>> something like: >>>> >>>> parse_conf_val(opt, subopt, str, &err); >>>> if (err) report_invalid_line(current_line); >>>> >>>> Thoughts? >>> Just my take: I prefer we do the right thing from the start. Even >>> if it takes us ages to move forward, baby steps, and clean patches >>> and evolutions, moving slowly away from the old crazy habits. >> In which case, I would just continue as we do now (terminating on an error), >> and then change it in the whole mkfs at once in some other patch set. There >> always will be something old and ugly we have to use temporarily, or we end >> up stashing one patchset after another, always trying to fix some other >> thing first, and never really fixing anything. > It seems like a change which could be fit into your series, it is more work, > but then again most of the work here was expected to be significant and doing > away with a lot of crap. Yes, I just don't think it match the focus of this specific set. > > Up to you, I just am providing my own feedback and making clear the requirements Thanks :-) > for the config stuff. I think it will be silly to add config support without > having the ability to catch errors and then indicate on its own however it > chooses exactly where the error occurred. I fully agree. We differ only in the opinion if it is important to do it right now, or if it would be better as a standalone change. > Will you take on doing the error code changes after ? > I put it into my todo list. Maybe it is not going to be the very next patch set, but it is there. Jan -- 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, Aug 03, 2017 at 03:23:24PM +0200, Jan Tulak wrote: > On 02/08/2017 21:48, Luis R. Rodriguez wrote: > > I think it will be silly to add config support without > > having the ability to catch errors and then indicate on its own however it > > chooses exactly where the error occurred. > I fully agree. We differ only in the opinion if it is important to do it > right now, or if it would be better as a standalone change. Not really, I don't care when it goes in so long as there is commitment to follow up to do that missing piece of work. The config file parsing thing can wait until this is all done, but it depends on it. > > Will you take on doing the error code changes after ? > > > I put it into my todo list. Maybe it is not going to be the very next patch > set, but it is there. OK cool! Then sure! 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 9d2db2a2..e008b5a4 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -117,6 +117,13 @@ unsigned int sectorsize; * Filled raw string from the user, so we never lose that information e.g. * to print it back in case of an issue. * + * value OPTIONAL + * The actual value used in computations for creating the filesystem. + * This is initialized with sensible default values. If initialized to 0, + * the value is considered disabled. User input will override default + * values. If the field is a string and not a number, the value is set to + * a positive non-zero number when user input has been supplied. + * */ struct opt_params { int index; @@ -134,6 +141,7 @@ struct opt_params { long long maxval; long long flagval; const char *raw_input; + uint64_t value; } subopt_params[MAX_SUBOPTS]; } opts[MAX_OPTS] = { #define OPT_B 0 @@ -749,6 +757,23 @@ struct opt_params { */ #define WHACK_SIZE (128 * 1024) +/* + * Get and set values to the opts table. + */ +static inline uint64_t +get_conf_val(int opt, int subopt) +{ + return opts[opt].subopt_params[subopt].value; +} + +static void +set_conf_val(int opt, int subopt, uint64_t val) +{ + struct subopt_param *sp = &opts[opt].subopt_params[subopt]; + + sp->value = val; +} + static inline void set_conf_raw(int opt, int subopt, const char *value) { @@ -880,6 +905,18 @@ getnum( } /* + * A wrapper for getnum and set_conf_val. + */ +static inline uint64_t +parse_conf_val(int opt, int subopt, char *value) +{ + uint64_t num = getnum(value, &opts[opt], subopt); + + set_conf_val(opt, subopt, num); + return num; +} + +/* * Convert lsu to lsunit for 512 bytes blocks and check validity of the values. */ static void
This patch adds infrastructure that will be used in subsequent patches. The Value field is the actual value used in computations for creating the filesystem. This is initialized with sensible default values. If initialized to 0, the value is considered disabled. User input will override default values. If the field is a string and not a number, the value is set to a positive non-zero number when user input has been supplied. Add functions that can be used to get/set values to opts table. Signed-off-by: Jan Tulak <jtulak@redhat.com> --- Change: * update the description of commit/in code * merge with another patch, which adds the get/set functions. --- mkfs/xfs_mkfs.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)