Re: [Libguestfs] [PATCH nbdkit v2 5/5] vddk: Implement parallel thread model
On 10/28/21 20:23, Richard W.M. Jones wrote:
On Thu, Oct 28, 2021 at 07:55:39PM +0200, Laszlo Ersek wrote:
> On 10/28/21 16:24, Richard W.M. Jones wrote:
>> +#ifdef HAVE_STDATOMIC_H
>> +#include <stdatomic.h>
>> +#else
>> +/* Some old platforms lack atomic types, but this is only used for
>> + * debug messages.
>> + */
>> +#define _Atomic /**/
>> +#endif
...
>> +/* Send command to the background thread and wait for completion.
>> + *
>> + * Returns 0 for OK
>> + * On error, calls nbdkit_error and returns -1.
>> + */
>> +int
>> +send_command_and_wait (struct vddk_handle *h, struct command *cmd)
>> +{
>> + static _Atomic uint64_t id = 0;
>
> Ugh.
>
> _Atomic is from C11, and it comes with a huge theoretical baggage
> (section "5.1.2.4 Multi-threaded executions and data races"). Do we
> really need this? For example:
[theoretical stuff snipped]
Sure thing, I'll put it in the handle structure and protect it with
the command queue lock.
I think I'll push this with the above change.
Thank you!
There are quite a few other cases of _Atomic in nbdkit & libnbd if
you're feeling spirited enough to check any of them :-)
Ouch, I didn't know!
I feel it's better if we can avoid this new instance. But messing with
otherwise working code (just for the sake of purity) is always risky...
It costs review time, plus whoever "cleans up" the code becomes
responsible for any regressions introduced by the "cleanup". I think
it's better if I check those parts if / when they emerge in review anyway.
Thanks!
Laszlo
Thanks,
Rich.