Re: [Libguestfs] [PATCH nbdkit 5/5] vddk: Implement parallel thread model
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