LibAliasSetMode error was not detected correctly

[rlibby]

Summary

Clang issued -Wtautological-compare for the check of the return value.

Test Plan

buildkernel

[mav]

I don't think this is a correct solution. It looks more like a workaround. Correct IMHO would be to make LibAliasSetMode() return int instead of unsigned it.

[rlibby]

Hmm. This procedure is exported in userspace libalias, is it okay to change its signature like that? Would you want the flags and mask arguments also changed from unsigned int to int for consistency?

The return type is awkward because it is used to return both flags and error. (This seems to be the only call site which checks for an error, the others either ignore the return entirely or use the return value as flags without checking for error.)

[mav]

In D12020#249277, @rlibby wrote:

Hmm. This procedure is exported in userspace libalias, is it okay to change its signature like that?

It is also documented in man page. So may be changing it is not so good idea.

I have another idea. Since this function returns resulted flags, it may be possible to make it return as disabled function which has failed to enable. In such case it would both stay compatible with old code and fixed the compilation errors like that, but slightly complicated check for enable error (if somebody really want it).

[rlibby]

In D12020#249291, @mav wrote:

I have another idea. Since this function returns resulted flags, it may be possible to make it return as disabled function which has failed to enable. In such case it would both stay compatible with old code and fixed the compilation errors like that, but slightly complicated check for enable error (if somebody really want it).

Ah, you mean to indicate error by returning with the flag not set in the return flags? Yeah, the error check is a little ugly, but I think that works.

[mav]

In D12020#249292, @rlibby wrote:

In D12020#249291, @mav wrote:

I have another idea. Since this function returns resulted flags, it may be possible to make it return as disabled function which has failed to enable. In such case it would both stay compatible with old code and fixed the compilation errors like that, but slightly complicated check for enable error (if somebody really want it).

Ah, you mean to indicate error by returning with the flag not set in the return flags? Yeah, the error check is a little ugly, but I think that works.

Right. From some degree it can be called more flexible. ;)

[rlibby]

LibAliasSetMode: mav feedback, indicate error via return flags

[mav]

sys/netinet/libalias/alias_db.c

2585

Wouldn't it be better to do something like:

mask &= ~PKT_ALIAS_LOG

and let processing of other options continue?

[rlibby]

sys/netinet/libalias/alias_db.c

2585

I think it's viable, if you would like. One way the mode setting has no effect on error, the other way it has a partial effect. I don't have a strong feeling about it. In practice continuing on error may work better for the clients which aren't checking for error anyway.

[mav]

sys/netinet/libalias/alias_db.c

2585

I think it would be more resilient to errors. But original way is also acceptable for me if you don't like this.

[rlibby]

sys/netinet/libalias/alias_db.c

2585

I would vote for LibAliasSetMode to have no effect on error. However, I am not deeply invested in this code, so if the other way seems better, I'm happy to defer judgement.

[mav]

sys/netinet/libalias/alias_db.c

2585

I think in general case it may be impossible to make it atomic in case error happen while handling not the first option but some later. On the other side, there is no exact error reported, so not sure it should be atomic. I also haven't touched that code for years, so don't care too much.

[rlibby]

sys/netinet/libalias/alias_db.c

2585

Okay :) I suggest we go with the behavior in the current patch then, as that should preserve the status quo with respect to the mode setting after an error enabling logging. I agree with your comment about the general case.