On Mon, Jun 23, 2025 at 06:47:05PM +0800, Su Hui wrote:
> There are three cleanups for vmcore_add_device_dump(). Adjust data_size's
> type from 'size_t' to 'unsigned int' for the consistency of data->size.
> Return -ENOMEM directly rather than goto the label to simplify the code.
> Using scoped_guard() to simplify the lock/unlock code.
>
> Signed-off-by: Su Hui <su...@nfschina.com>
> ---
> fs/proc/vmcore.c | 33 ++++++++++++++-------------------
> 1 file changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index 10d01eb09c43..9ac2863c68d8 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -1477,7 +1477,7 @@ int vmcore_add_device_dump(struct vmcoredd_data *data)
> {
> struct vmcoredd_node *dump;
> void *buf = NULL;
> - size_t data_size;
> + unsigned int data_size;
> int ret;
This was in reverse Christmas tree order before. Move the data_size
declaration up a line.
long long_variable_name;
medium variable_name;
short name;
>
> if (vmcoredd_disabled) {
> @@ -1490,10 +1490,8 @@ int vmcore_add_device_dump(struct vmcoredd_data *data)
> return -EINVAL;
>
> dump = vzalloc(sizeof(*dump));
> - if (!dump) {
> - ret = -ENOMEM;
> - goto out_err;
> - }
> + if (!dump)
> + return -ENOMEM;
>
> /* Keep size of the buffer page aligned so that it can be mmaped */
> data_size = roundup(sizeof(struct vmcoredd_header) + data->size,
> @@ -1519,21 +1517,18 @@ int vmcore_add_device_dump(struct vmcoredd_data *data)
> dump->size = data_size;
>
> /* Add the dump to driver sysfs list and update the elfcore hdr */
> - mutex_lock(&vmcore_mutex);
> - if (vmcore_opened)
> - pr_warn_once("Unexpected adding of device dump\n");
> - if (vmcore_open) {
> - ret = -EBUSY;
> - goto unlock;
> - }
> -
> - list_add_tail(&dump->list, &vmcoredd_list);
> - vmcoredd_update_size(data_size);
> - mutex_unlock(&vmcore_mutex);
> - return 0;
> + scoped_guard(mutex, &vmcore_mutex) {
> + if (vmcore_opened)
> + pr_warn_once("Unexpected adding of device dump\n");
> + if (vmcore_open) {
> + ret = -EBUSY;
> + goto out_err;
> + }
>
> -unlock:
> - mutex_unlock(&vmcore_mutex);
> + list_add_tail(&dump->list, &vmcoredd_list);
> + vmcoredd_update_size(data_size);
> + return 0;
Please, move this "return 0;" out of the scoped_guard(). Otherwise
it's not obvious that we return zero on the success path.
regards,
dan carpenter
> + }
>
> out_err:
> vfree(buf);
> --
> 2.30.2
>
