[Libguestfs] [PATCH nbdkit 5/5] vddk: Implement parallel thread model

[Richard W.M. Jones]

On Wed, Oct 27, 2021 at 07:42:31PM +0200, Laszlo Ersek wrote:
> On 10/27/21 14:21, Richard W.M. Jones wrote:
> > +/* Queue of asynchronous commands sent to the background thread. */
> > +enum command_type { GET_SIZE, READ, WRITE, FLUSH, CAN_EXTENTS, EXTENTS, STOP };
> 
> (1) Shouldn't we use a common prefix for these enum constants?

If we get a conflict with a header then yes.  For example I was
disappointed a while back to see that I couldn't create a local
variable called (IIRC) "socket" (or maybe "connect") - which is
ridiculous!  If it's a symbol that needs to be namespaced (eg. it's
part of a public API) then absolutely required.  But the rest of the
time ... I'm not that bothered, saves typing.

> > +  /* Add the command to the command queue. */
> > +  {
> > +    ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&h->commands_lock);
> > +    r = command_queue_append (&h->commands, cmd);
> > +    if (r == 0)
> > +      pthread_cond_signal (&h->commands_cond);
> > +  }
> 
> (*shudder* -- the automatic releasing of the mutex, upon exiting the
> scope, combined with the explicit signaling of the associated condvar,
> is terrible to read. Functionally it is correct, though.)

It's a bit awkward in this particular case, but I think this automatic
releasing of mutexes in general is *great*.  In many cases you put the
macro at the top of the function, fire and forget.  No need to
painfully clean up on every exit path.

Of course it does require a C extension ...

> (4) This could be slightly optimized by restricting the condvar signal
> to "queue size is now exactly 1 (i.e., it was empty)".

I think there's a possible race with that, but let me think about it.

> (11a) The switch statement is enormous. Each branch should have a
> dedicated helper function.

Yup, I did that change already, makes it a lot easier to follow :-)

I'll follow up on the rest in the v2 patch.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top