Re: [PATCH: glusterfs] fixed? alu

[Amar Tumballi <amar@xxxxxxxxxxx>]

 This patch includes two smaller things:
 1. support to define an option of type PERCENT_OR_SIZET, (modification in libglusterfs/src/*)

 2. support in alu to provide min-free-disk option with type PERCENT_OR_SIZET 


Review replies inline:

On Wed, Apr 22, 2009 at 4:10 PM, Paul Rawson <plrca2@xxxxxxxxx> wrote:

Sorry, I think gmail is screwing up the formatting. Let me know if
this still isn't right.

formatting is fine now.

+       case GF_OPTION_TYPE_PERCENTORSIZET:
+       {
+               uint32_t percent = 0;
+               uint32_t input_size = 0;
+
+               /* Check if the value is valid percentage */
+               if (gf_string2percent (pair->value->data,
+                                      &percent) == 0) {
+                       if ((percent < 0) || (percent > 100)) {
+                               gf_log (xl->name, GF_LOG_ERROR,
+                               "'%d' in 'option %s %s' is out of "
+                               "range [0 - 100]",
+                               percent, pair->key,
+                               pair->value->data);
+                       }
+                       ret = 0;
+                       goto out;

assume a user gives "option min-free-disk 131072" (with the intention of disk size in size_t), gf_string2percent returns 0, but the check to validate the range fails. but it won't pass the check for 'gf_string2bytesize in this case. So, even logging 'error' in that case won't be valid.

+               } else {
+                       /* Check the range */
+                       if (gf_string2bytesize (pair->value->data,
+                                               &input_size) == 0) {
+
+                              if ((opt->min == 0) && (opt->max == 0)) {
+                                       gf_log (xl->name, GF_LOG_DEBUG,
+                                               "no range check required for "
+                                               "'option %s %s'",
+                                               pair->key, pair->value->data);
+                                       ret = 0;
+                                       break;
+                               }
+                               if ((input_size < opt->min) ||
+                                   (input_size > opt->max)) {
+                                       gf_log (xl->name, GF_LOG_ERROR,
+                                               "'%"PRId64"' in 'option %s %s' is "
+                                               "out of range [%"PRId64" - %"PRId64"]",
+                                               input_size, pair->key,
+                                               pair->value->data,
+                                               opt->min, opt->max);
+                               }
+                               ret = 0;
+                               goto out;
+                       } else {
+                               // It's not a percent or size
+                               gf_log (xl->name, GF_LOG_ERROR,
+                               "invalid number format \"%s\" "
+                               "in \"option %s\"",
+                               pair->value->data, pair->key);
+
+                       }
+               }
+       }
+       break;

-- 
Amar Tumballi