Re: [PATCH v2 5/6] tools/misc: xenwatchdogd enhancements

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

On Fri, Mar 29, 2024 at 11:10:55AM +0000, leigh@xxxxxxxxxxxxx wrote:
> From: Leigh Brown <leigh@xxxxxxxxxxxxx>
> 
> Add enhanced parameter parsing and validation, making use of
> getopt_long(). Adds usage() function, ability to run in the foreground,
> and the ability to disarm the watchdog timer when exiting.  Now checks
> the number of parameters are correct, that timeout is at least two
> seconds (to allow a minimum sleep time of one second), and that the
> sleep time is at least one and less than the watchdog timeout. After
> these changes, the daemon will no longer instantly reboot the domain
> if you enter a zero timeout (or non-numeric parameter), and prevent
> the daemon consuming 100% of a CPU. Add a copyright message. This is
> based on the previous commits which were from Citrix email addresses.

This to me is really hard to read, it just looks like a blob of text,
where it supposed to be a list with several modification listed. The
part about the copyright should be in its own paragraph for example.


> Signed-off-by: Leigh Brown <leigh@xxxxxxxxxxxxx>
> ---
>  tools/misc/xenwatchdogd.c | 111 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 101 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/misc/xenwatchdogd.c b/tools/misc/xenwatchdogd.c
> index 19ec4c5359..b78320f86d 100644
> --- a/tools/misc/xenwatchdogd.c
> +++ b/tools/misc/xenwatchdogd.c
> @@ -1,3 +1,20 @@
> +/*
> + * xenwatchdogd.c
> + *
> + * Watchdog based on Xen hypercall watchdog interface
> + *
> + * Copyright 2010-2024 Citrix Ltd and other contributors

This is probably more like:
Copyright (C) 2010 Citrix Ltd.
Copyright (C) 2024 *** your copyright here ***

Because it's looks like the only contribution from us was in 2010, and I
suppose it's fine to have more than one copyright line.

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.

This might be the wrong license, the default license we use is GPL 2.0
only, not LGPL. See :/COPYING .

These days, we prefer SPDX tags instead of the full licence text.

So overall, the header of the file should look something like:

/* SPDX-License-Identifier: GPL-2.0-only */
/*
 * xenwatchdogd.c
 * 
 * Watchdog based on Xen hypercall watchdog interface
 * 
 * Copyright (C) 2010 Citrix Ltd.
 * Copyright (C) 2024 *** your copyright here ***
 */

I don't know if adding the file name in that header is very useful, but
I don't mind either way.

Also, could you do this in a separate patch?

> + */
>  
>  #include <err.h>
>  #include <limits.h>

Nice change overall, it's just the license part that need fixing.

Thanks,

-- 
Anthony PERARD