Fix some leaks in usr.sbin/bhyve/block_if.c

[slw_zxy.spb.ru]

Summary

Some resource allocated and not correctly freed in usr.sbin/bhyve/block_if.c.

[novel]

usr.sbin/bhyve/block_if.c

681

What are the scenarios where we end up in the err: label with the bc already allocated? Last time we jump to the err label is when bc allocation fails, so still nothing to clean up at this point, and then we don't seem to end up there at all.

878

I think style(9) says to declare variables before statements.

[slw_zxy.spb.ru]

usr.sbin/bhyve/block_if.c

681

In internall repo we have additional init before creating blockif_thr threads.

878

I see, will be fixed.

[bnovkov]

usr.sbin/bhyve/block_if.c

682–684

Unfortunately we have no way of knowing whether these were successfully created since we don't check for the return values of pthread_{mutex,cond}_* calls above. Could you please add those checks and adjust the cleanup code accordingly?

[glebius]

usr.sbin/bhyve/block_if.c

681

Without your internal changes the diff looks strange. Why not to free resources right in the block where if (bc == NULL) {? In case you are not going to upstream your other changes, the best way would be to make the upstreamed change styled correctly: destroy resources right after calloc() failure, then refactor your internal changes upon that. For example you can goto up the code to fall into the beginning of the if (bc == NULL) { block.

682–684

And pthread_create, which is way more likely to fail.

NB: This function may benefit from __attribute__((cleanup)). I don't know if we already have a precedence of using it in src.

[slw_zxy.spb.ru]

Check pthread_create/pthread_mutex_init/pthread_mutex_init

[slw_zxy.spb.ru]

The patch is updated to match all your comments. What needs to be done from my side to get it pushed to main?

[bnovkov]

usr.sbin/bhyve/block_if.c

656–657

calloc will zero this array so this loop and the NULL assignments are not necessary.

688

i is already defined at the start of the function, this won't compile.

689–696

Please compare the pointers in if conditions to NULL, there are a few places in this patch where this should be changed.

[slw_zxy.spb.ru]

Fix compilation issue and eliminate loop.

[slw_zxy.spb.ru]

The patch is updated to match all your last comments. What needs to be done from my side to get it pushed to main?

[bnovkov]

In D57887#1333560, @slw_zxy.spb.ru wrote:

The patch is updated to match all your last comments. What needs to be done from my side to get it pushed to main?

As far as I'm concerned everything looks ok now, I'll land the change tomorrow.

[bnovkov]

usr.sbin/bhyve/block_if.c

900–901

One question before I land this, what was your rationale for moving the struct blockif_sig_elem bse; and its mutex/condvar initialization out of the while loop?
It would make more sense to initialize and destroy the mutex/condvar inside the loop.
I can fix this before I land the change.

[slw_zxy.spb.ru]

usr.sbin/bhyve/block_if.c

900–901

Inside loop bse acting as automatic variable and must be initialzing and destroing each time through the loop iteration. This is more complex (IMHO) and more expensive.