cite your reference
You didn't mention any documentation URLs.
It would really help the review context if you explain what a ZCL
(zigbee cluster?) or a ZCL3B is.
use braces
Even a single-line if
body deserves {
}
braces.
Sooner or later, someone is going to maintain this code.
It might not be you.
They might fall into the trap you've laid for them.
compound type
This just isn't enough:
#define ZCL3B_LOCK_FLAG 0x80
You really need to /* explain */ that you're adding 128 to
what would otherwise be a valid buffer index.
That is, we have {flag_bit, five_padding_zeros, index}.
As written, I would expect this to go into a "flags byte".
Alternatively, consider defining a 0x3
mask on the subsequent
line, again with an eye on explaining the fields together in one place.
It is nice that you've grouped this with zcl3b_idx_t
, thank you.
I just had no idea they were deliberately grouped, upon my first reading.
magic buf len
typedef struct {
uint32_t data[128];
} msg_t;
...
for (int i = 1; i < sizeof(msg->data) / sizeof(msg->data[0]); i++) {
...
for (int i = 0; i < sizeof(msg->data) / sizeof(msg->data[0]); i++)
The magic number 128
appears in three places,
albeit two of them will be automatically dealt with
if a maintainer e.g. doubles the buffer size.
Better to define a constant for the number of entries.
don't ignore errors
static int create_shm(size_t buf_size)
{
...
int r = memfd_create("zcl3b", MFD_ALLOW_SEALING);
if (r < 0)
return -errno;
...
int main()
{
int fd = create_shm(sizeof(msg_t));
pid_t pid = fork();
and then eventually zcl3b_map_init
will notice the negative file descriptor.
Prefer to deal with errors where they happen,
rather than letting them cascade down the call stacks
of two separate processes.
The contracts
of these functions would then be much simpler.
If we write bugs, we want to write shallow bugs
that some poor maintainer down the road can easily diagnose.
The goto fail
is very nice, it's a standard pattern.
Dealing with other failed sys calls, such as fork(), would also be good.
synchronous writes at init
atomic_store_explicit(map->xchg, BUFFER_NONE, memory_order_relaxed);
This achieves atomic writes with
relaxed_ordering
where we probably wanted sequential consistency across processes.
It only runs once per process so it would be hard to observe race lossage.
one more stat
msg_t *msg = zcl3b_consumer_update(&consumer);
if (msg == NULL) {
usleep(1000);
continue;
}
In consumer_task
, consider incrementing a separate statistic there.
(I suppose you already have it, if you sum statistics and
subtract that from NUM_CYCLES.)
Alternatively, consider writing a "wait for producer startup" loop
that will only let us continue on to the main for
loop
once it has seen non-zero data from the producer.
torture test
The point of this code is to make a convincing argument
that consumer never sees unlocked (inconsistent) data.
So consider performing the verification reads according
to some shuffled permuted order, rather than sequentially.
Or more simply, do the reads in reverse order.
This code achieves its design goals.
I would be willing to delegate or accept maintenance tasks on it.