On Fri, Jan 07, 2022 at 02:39:17PM +0100, Michal Privoznik wrote:
> The g_strsplit() function can return NULL if and only if either
> the input string is NULL or delimiter is NULL or an empty string.
> In neither of places we call it any of the conditions is true and
> thus we don't need to check for the return value.
>
> Signed-off-by: Michal Privoznik <mpriv...@redhat.com>
> ---
> src/libxl/xen_xl.c | 2 --
> src/util/vircgroupv2.c | 2 --
> src/util/virprocess.c | 2 --
> src/util/virresctrl.c | 2 --
> 4 files changed, 8 deletions(-)
>
> diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
> index 7604e3d534..869083a1d1 100644
> --- a/src/libxl/xen_xl.c
> +++ b/src/libxl/xen_xl.c
> @@ -265,8 +265,6 @@ xenParseXLCPUID(virConf *conf, virDomainDef *def)
> }
>
> cpuid_pairs = g_strsplit(cpuid_str, ",", 0);
> - if (!cpuid_pairs)
> - return -1;
>
> if (!cpuid_pairs[0])
> return 0;
But the following pattern:
if (!(identifier = g_strsplit(instr, delim, N)))
return -1;
can be found in many more places and it is semantically equivalent to what
you're removing in your patch, so all of those should be adjusted. While it
is true that NULL is returned only under those 3 conditions you mentioned,
I don't think this patch is really desirable as a simple typo like "" for the
delimiter which can go unnoticed easily in a large patch series is a problem
which may or may not pop up immediately depending on what tests have been
performed that would exercise the code.
So, unless you have a very compelling point on why we'd benefit from this patch
it's a NACK from me.
Erik
