Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1255760

Re: [PATCH] staging:lustre: Prevent duplicate CT registrations

Show all headers | View raw

On Fri, Oct 23, 2015 at 03:59:14PM -0400, James Simmons wrote:
> diff --git a/drivers/staging/lustre/lustre/lmv/lmv_obd.c b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
> index 635a93c..d6d70d8 100644
> --- a/drivers/staging/lustre/lustre/lmv/lmv_obd.c
> +++ b/drivers/staging/lustre/lustre/lmv/lmv_obd.c
> @@ -794,7 +794,9 @@ static void lmv_hsm_req_build(struct lmv_obd *lmv,
>  static int lmv_hsm_ct_unregister(struct lmv_obd *lmv, unsigned int cmd, int len,
>  				 struct lustre_kernelcomm *lk, void *uarg)
>  {
> -	int	i, rc = 0;
> +	struct kkuc_ct_data *kcd = NULL;
> +	int rc = 0;
> +	__u32 i;

We have been introducing a lot of new __u32 types here and I just
assumed there was a reason for it but this one is clearly wrong.  The
new code implies that ->ld_tgt_count can overflow INT_MAX which is not
true and that this is code shared with userspace which might be true but
it's not described in the changelog.  Is this a static checker fix?
Stop using that broken static checker, because the correct type here is
int.

Anyway, stop making gratuitous unrelated changes (like the white space
changes to local declarations).  I feel like I have held off commenting
on this for a while and shown great restraint.  :P

> -	rc = libcfs_kkuc_group_rem(lk->lk_uid, lk->lk_group);
> +	rc = libcfs_kkuc_group_rem(lk->lk_uid, lk->lk_group, (void**)&kcd);
> +	if (kcd != NULL)
> +		kfree(kcd);

NULL check not needed.

> +
>  	return rc;
>  }
>  
>  static int lmv_hsm_ct_register(struct lmv_obd *lmv, unsigned int cmd, int len,
>  			       struct lustre_kernelcomm *lk, void *uarg)
>  {
> -	struct file	*filp;
> -	int		 i, j, err;
> -	int		 rc = 0;
> -	bool		 any_set = false;
> +	struct file *filp;
> +	__u32 i, j;
> +	int err, rc = 0;
> +	bool any_set = false;
> +	struct kkuc_ct_data *kcd;
>  
>  	/* All or nothing: try to register to all MDS.
>  	 * In case of failure, unregister from previous MDS,
> @@ -854,12 +860,25 @@ static int lmv_hsm_ct_register(struct lmv_obd *lmv, unsigned int cmd, int len,
>  
>  	/* at least one registration done, with no failure */
>  	filp = fget(lk->lk_wfd);
> -	if (filp == NULL) {
> +	if (filp == NULL)
>  		return -EBADF;
> -	}
> -	rc = libcfs_kkuc_group_add(filp, lk->lk_uid, lk->lk_group, lk->lk_data);
> -	if (rc != 0 && filp != NULL)
> +
> +	kcd = kzalloc(sizeof(*kcd), GFP_NOFS);
> +	if (kcd == NULL) {
>  		fput(filp);
> +		return -ENOMEM;
> +	}
> +	kcd->kcd_magic = KKUC_CT_DATA_MAGIC;
> +	kcd->kcd_uuid = lmv->cluuid;
> +	kcd->kcd_archive = lk->lk_data;
> +
> +	rc = libcfs_kkuc_group_add(filp, lk->lk_uid, lk->lk_group, kcd);
> +	if (rc != 0) {

These double negatives are a pet peev of mine.  "if (rc) {"  Comparing
with zero like this is idiomatic when you're talking about the number
zero or strcmp().  Can we use a goto for unwinding?  goto free_kcd;

> +		if (filp != NULL)

The earlier NULL check means this can't happen.

> +			fput(filp);
> +		kfree(kcd);
> +	}
> +
>  	return rc;

	return 0;

free_kcd:
	kfree(kcd);
put_filp:
	fput(filp);

	return rc;

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/5] staging:lustre: split kernel comm between user and kernel James Simmons <jsimmons@infradead.org> - 2015-10-23 22:00 +0200
  [PATCH] staging:lustre: Prevent duplicate CT registrations James Simmons <jsimmons@infradead.org> - 2015-10-23 22:10 +0200
    Re: [PATCH] staging:lustre: Prevent duplicate CT registrations Dan Carpenter <dan.carpenter@oracle.com> - 2015-10-26 10:00 +0100
  [PATCH 1/5] staging:lustre: kg_sem semaphore handling is incorrectly James Simmons <jsimmons@infradead.org> - 2015-10-23 22:10 +0200
  [PATCH 4/5] staging:lustre: split kernel comm between user and kernel James Simmons <jsimmons@infradead.org> - 2015-10-23 22:10 +0200
  [PATCH] staging:lustre: move kernel_user_comm.c from libcfs to lustre James Simmons <jsimmons@infradead.org> - 2015-10-23 22:10 +0200
  [PATCH 5/5] staging:lustre: Update license and copyright for kernel comm James Simmons <jsimmons@infradead.org> - 2015-10-23 22:10 +0200
  Re: [PATCH 0/5] staging:lustre: split kernel comm between user and  kernel Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-10-25 09:40 +0100

csiph-web