Reviews inline:<br><div class="gmail_quote"><div><br>This patch has dependency on previous/another patch on alu's disk-usage parsing. <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
---<br>
libglusterfs/src/xlator.c | 2 +-<br>
scheduler/alu/src/alu.c | 8 +++--<br>
xlators/cluster/dht/src/dht-common.h | 3 +-<br>
xlators/cluster/dht/src/dht-diskusage.c | 48 +++++++++++++++++++++++--------<br>
xlators/cluster/dht/src/dht.c | 24 +++++++++------<br>
xlators/cluster/dht/src/nufa.c | 24 ++++++++++-----<br>
6 files changed, 75 insertions(+), 34 deletions(-)<br>
<br>
diff --git a/libglusterfs/src/xlator.c b/libglusterfs/src/xlator.c<br>
index 5b5067a..0345037 100644<br>
--- a/libglusterfs/src/xlator.c<br>
+++ b/libglusterfs/src/xlator.c<br>
@@ -434,7 +434,7 @@ _volume_option_value_validate (xlator_t *xl,<br>
case GF_OPTION_TYPE_PERCENTORSIZET:<br>
{<br>
uint32_t percent = 0;<br>
- uint32_t input_size = 0;<br>
+ input_size = 0;<br>
<br>
/* Check if the value is valid percentage */<br>
if (gf_string2percent (pair->value->data,<br>
diff --git a/scheduler/alu/src/alu.c b/scheduler/alu/src/alu.c<br>
index 8939531..967eece 100644<br>
--- a/scheduler/alu/src/alu.c<br>
+++ b/scheduler/alu/src/alu.c<br>
@@ -98,6 +98,7 @@ get_stats_free_disk (struct xlator_stats *this)<br>
return this->free_disk;<br>
}<br>
}<br>
+ return 0;<br>
}<br>
<br>
static int64_t<br>
@@ -384,10 +385,11 @@ alu_init (xlator_t *xl)<br>
_limit_fn->next = tmp_limits;<br>
alu_sched->limits_fn = _limit_fn;<br>
<br>
- if (gf_string2percent (limits->data, &min_free_disk) == 0) {<br>
- alu_sched->spec_limit.disk_unit = 'p';<br>
- } else {<br>
+ if (gf_string2bytesize (limits->data, &min_free_disk)) {<br>
alu_sched->spec_limit.disk_unit = 'b';<br>
+ } else {<br>
+ gf_string2percent (limits->data, &min_free_disk);<br>
+ alu_sched->spec_limit.disk_unit = 'p';<br>
}<br>
</blockquote><div><br>In all the places its logical to check for percent first and if fails then to look for bytesize.<br><br>Imagine a case where 'option min-free-disk 90' is given. (note that there is no % or 'MB/GB/TB' after 90).<br>
<br>User's intention would be set it to 'percent' based check, but both parser and translators assume it as bytesize and consider 90bytes as the limit to treat a disk as full :O<br><br>Other than this change (everywhere the logic is used), patch is good. <br>
<br>
+ gf_string2percent (limits->data, &min_free_disk);<br>
<br>'gf_string2percent() takes 'int32_t *' as second argument, hence it gives warning which will make our build scripts to fail. Instead you can use a temporary variable there, and initialize the min_free_disk variable.<br>
<br>Paul, can you resend the patch ? or do you want us to make this change and commit?<br><br>Thanks and Regards,<br><br></div></div>-- <br>Amar Tumballi<br><br>