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

[Richard W.M. Jones]

On Mon, Apr 20, 2020 at 09:24:43AM -0500, Eric Blake wrote:
> 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.

OK, I'll remove the comment from the commit message.  I was
wondering why it didn't obviously crash :-)

> At any rate, the series looks good.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v