Re: [Libguestfs] [PATCH nbdkit 2/2] Add insert function and use the new vector library in several places.

On 4/19/20 4:14 PM, Richard W.M. Jones wrote: >This extends the vector library with an insert function. It is more >expensive than appending, but this does not affect existing code using >vector and can be used in new places without making those uses more >expensive. > >We use this function in nbdkit-extentlist-filter, nbdkit-eval-plugin >and the sparse library. > >This is refactoring, so should not affect functionality at all. >However during the rewrite of eval it revealed a fencepost error in >the loop iterating over method_scripts which is fixed here. >--- > common/sparse/Makefile.am | 1 + > common/utils/vector.h | 24 +++++-- > common/sparse/sparse.c | 50 +++++++------- > plugins/eval/eval.c | 49 ++++++-------- > filters/extentlist/extentlist.c | 113 ++++++++++++++++---------------- > TODO | 5 +- > 6 files changed, 119 insertions(+), 123 deletions(-) >+++ b/plugins/eval/eval.c >@@ -104,21 +106,12 @@ compare_script (const void *methodvp, const void *entryvp) > static int > insert_method_script (const char *method, char *script) > { >- struct method_script *newp; >- size_t i; > int r; >+ size_t i; >+ struct method_script new_entry = { .method = method, .script = script }; >- newp = realloc (method_scripts, >- sizeof (struct method_script) * (nr_method_scripts + 1)); >- if (newp == NULL) { >- nbdkit_error ("insert_method_script: realloc: %m"); >- return -1; >- } >- nr_method_scripts++; >- method_scripts = newp; The old code appended the new element first, then >- >- for (i = 0; i < nr_method_scripts-1; ++i) { >- r = compare_script (method, &method_scripts[i]); iterated on the previously-existing elements to see if re-sorting was needed. >+ for (i = 0; i < method_scripts.size; ++i) { >+ r = compare_script (method, &method_scripts.ptr[i]); The new code does not increase the list size until first finding where to insert. I see why you called this a fencepost error, as comparing just the pre-/post-patch 'for' lines looks like you fixed an off-by-one error, but with the additional context of when the list counter was incremented, I don't think it is actually a bug fix, so the commit message had me hunting for something not there.

At any rate, the series looks good.