[PATCH 1/1] Checking the value returned by the function
Michal Prívozník
mprivozn at redhat.com
Fri Oct 20 09:00:07 UTC 2023
On 10/11/23 16:31, Sergey Mironov wrote:
> In version 0.10.0-rc0 (https://github.com/libvirt/libvirt/blob/v0.10.0-rc0/src/security/security_selinux.c ) 11 years ago,
> the virSecuritySELinuxSetFileconHelper function appeared, which returned 1 if the optional is true.
> Considering that at the moment the virSecuritySELinuxSetFilecon function (by definition) can only return 0 or -1, I suggest removing the "dead code" in the current patch.
>
> Co-developed-by: sdl.qemu <sdl.qemu at linuxtesting.org>
> Signed-off-by: Sergey Mironov <mironov at fintech.ru>
> ---
> src/security/security_selinux.c | 11 -----------
> 1 file changed, 11 deletions(-)
>
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 7914aba84d..a7abab9cf8 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1988,17 +1988,6 @@ virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr,
> ret = virSecuritySELinuxSetFilecon(mgr, path, use_label, remember);
> }
>
> - if (ret == 1 && !disk_seclabel) {
> - /* If we failed to set a label, but virt_use_nfs let us
> - * proceed anyway, then we don't need to relabel later. */
> - disk_seclabel = virSecurityDeviceLabelDefNew(SECURITY_SELINUX_NAME);
> - if (!disk_seclabel)
> - return -1;
> - disk_seclabel->labelskip = true;
> - VIR_APPEND_ELEMENT(src->seclabels, src->nseclabels, disk_seclabel);
> - ret = 0;
> - }
> -
> return ret;
> }
>
At this point, the @ret variable can be removed as well. But I can live
with that. What I can't live with is the commit message. Firstly,
there's no need to link the source file if you mention the actual commit
that introduced the function: 12b187fb956bffbc67a090dcda89e66450503a4e
and to make the hash more readable, just use git-describe (which in this
particular case yields some CVE-* tag, so pass --match=v\* to get
v0.10.0-rc0~108).
Nit-pick - mentioning the commit when a function was introduced is good,
but also, we are probably more interested in what commit turned these
lines into a dead code. Because without that the first piece of
information makes a great sensation (we have a dead code for 11 years!,
which is obviously not true).
Then there are more rigid customs we tend to hold:
1) if you look at 'git log --oneline' you'll see that majority (if not
all) commits are prefixed with something (usually the module they
change). Thus in this case it should have been "selinux: ".
2) Continuing with git log - "selinux: Checking the value ..." does not
really describe what's happening. You are dropping the check, not
introducing it.
I'm sorry if I sound too picky, but these rules help greatly downstream
maintainers when they need to figure out what each commit does.
Anyway, I'm fixing the commit message and pushing.
Reviewed-by: Michal Privoznik <mprivozn at redhat.com>
Michal
More information about the libvir-list
mailing list