crush/builder: fix ENOENT when removing last bucket item

We were decrementing size and then breaking out ENOENT condition check.
Fix by decrementing size only after we break out of the loop and verify
we found the item.

Fix a follow-on bug by avoiding realloc when we have 0 items left.  This case
was never exercised before due to the ENOENT issue; now we return explicitly.
It's really not necessary to realloc at all, probably, since these are very
small arrays, but in any case leaving a single item allocation there in place of
a 0-length allocation is fine.  (And the 0-length allocation behvaior on realloc
is undefined.. may either return a pointer or NULL.)

Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/crush/builder.c b/src/crush/builder.c
index 3b4ba25b277b4f35dc9c0816ec1783dbb629dedf..f492a27d3819b98125978e3c02c156f75c480737 100644 (file)
--- a/src/crush/builder.c
+++ b/src/crush/builder.c
@@ -1030,12 +1030,11 @@ int crush_remove_straw_bucket_item(struct crush_map *map,
 
        for (i = 0; i < bucket->h.size; i++) {
                if (bucket->h.items[i] == item) {
-                       bucket->h.size--;
                        if (bucket->item_weights[i] < bucket->h.weight)
                                bucket->h.weight -= bucket->item_weights[i];
                        else
                                bucket->h.weight = 0;
-                       for (j = i; j < bucket->h.size; j++) {
+                       for (j = i; j < bucket->h.size - 1; j++) {
                                bucket->h.items[j] = bucket->h.items[j+1];
                                bucket->item_weights[j] = bucket->item_weights[j+1];
                        }
@@ -1044,7 +1043,11 @@ int crush_remove_straw_bucket_item(struct crush_map *map,
        }
        if (i == bucket->h.size)
                return -ENOENT;
-       
+       bucket->h.size--;
+       if (bucket->h.size == 0) {
+               /* don't bother reallocating */
+               return 0;
+       }
        void *_realloc = NULL;
 
        if ((_realloc = realloc(bucket->h.items, sizeof(__s32)*newsize)) == NULL) {
@@ -1074,12 +1077,11 @@ int crush_remove_straw2_bucket_item(struct crush_map *map,
 
        for (i = 0; i < bucket->h.size; i++) {
                if (bucket->h.items[i] == item) {
-                       bucket->h.size--;
                        if (bucket->item_weights[i] < bucket->h.weight)
                                bucket->h.weight -= bucket->item_weights[i];
                        else
                                bucket->h.weight = 0;
-                       for (j = i; j < bucket->h.size; j++) {
+                       for (j = i; j < bucket->h.size - 1; j++) {
                                bucket->h.items[j] = bucket->h.items[j+1];
                                bucket->item_weights[j] = bucket->item_weights[j+1];
                        }
@@ -1089,6 +1091,12 @@ int crush_remove_straw2_bucket_item(struct crush_map *map,
        if (i == bucket->h.size)
                return -ENOENT;
 
+       bucket->h.size--;
+       if (!newsize) {
+               /* don't bother reallocating a 0-length array. */
+               return 0;
+       }
+
        void *_realloc = NULL;
 
        if ((_realloc = realloc(bucket->h.items, sizeof(__s32)*newsize)) == NULL) {