Messages in this thread | | | Date | Mon, 23 Jul 2007 22:59:25 +0200 | From | "Jesper Juhl" <> | Subject | Re: [DRIVER SUBMISSION] DRBD wants to go mainline |
| |
On 21/07/07, Lars Ellenberg <lars@linbit.com> wrote: > > DRBD wants to go mainline. > please have a look at the "for-linus" branch of > git://git.drbd.org/home/git/linux-drbd.git. > A few more comments.
In drbd_main.c::after_state_ch() there's this code :
... if ( mdev->p_uuid ) { kfree(mdev->p_uuid); mdev->p_uuid = NULL; } ...
Since kfree(NULL) if a noop, that should just become :
... kfree(mdev->p_uuid); mdev->p_uuid = NULL; ...
Same for these bits in drbd_main.c::drbd_cleanup()
... if(mdev->app_reads_hash) { kfree(mdev->app_reads_hash); mdev->app_reads_hash = NULL; } if ( mdev->p_uuid ) { kfree(mdev->p_uuid); mdev->p_uuid = NULL; } ...
They should just become
... kfree(mdev->app_reads_hash); mdev->app_reads_hash = NULL; kfree(mdev->p_uuid); mdev->p_uuid = NULL; ...
And then there's drbd_main.c::drbd_new_device() :
... if(mdev->app_reads_hash) kfree(mdev->app_reads_hash); ...
That should just be
... kfree(mdev->app_reads_hash); ...
Btw; You shouldn't put multiple statements on the same line. That is seen all over the place - like
mdev = kzalloc(sizeof(drbd_dev),GFP_KERNEL); if(!mdev) goto Enomem;
That should be
mdev = kzalloc(sizeof(drbd_dev),GFP_KERNEL); if(!mdev) goto Enomem;
There are lots of other examples.
Here are a few more pointless NULL checks before kfree() :
drbd_nl.c::drbd_nl_net_conf() : ... if(new_tl_hash) { if (mdev->tl_hash) kfree(mdev->tl_hash); mdev->tl_hash_s = mdev->net_conf->max_epoch_size/8; mdev->tl_hash = new_tl_hash; }
if(new_ee_hash) { if (mdev->ee_hash) kfree(mdev->ee_hash); mdev->ee_hash_s = mdev->net_conf->max_buffers/8; mdev->ee_hash = new_ee_hash; }
if ( mdev->cram_hmac_tfm ) { crypto_free_hash(mdev->cram_hmac_tfm); } mdev->cram_hmac_tfm = tfm; ...
should become :
... if(new_tl_hash) { kfree(mdev->tl_hash); mdev->tl_hash_s = mdev->net_conf->max_epoch_size/8; mdev->tl_hash = new_tl_hash; }
if(new_ee_hash) { kfree(mdev->ee_hash); mdev->ee_hash_s = mdev->net_conf->max_buffers/8; mdev->ee_hash = new_ee_hash; }
crypto_free_hash(mdev->cram_hmac_tfm); mdev->cram_hmac_tfm = tfm; ...
still in drbd_nl.c::drbd_nl_net_conf() :
... fail: if (tfm) crypto_free_hash(tfm); if (new_tl_hash) kfree(new_tl_hash); if (new_ee_hash) kfree(new_ee_hash); if (new_conf) kfree(new_conf); ...
should become
... fail: crypto_free_hash(tfm); kfree(new_tl_hash); kfree(new_ee_hash); kfree(new_conf); ...
drbd_nl.c::ensure_mdev() :
... if(mdev->app_reads_hash) kfree(mdev->app_reads_hash); ..
becomes
... kfree(mdev->app_reads_hash); ...
In drbd_receiver.c::receive_uuids() we find this :
... if ( mdev->p_uuid ) kfree(mdev->p_uuid); ...
which should be just
... kfree(mdev->p_uuid); ...
There is also drbd_receiver.c::drbd_disconnect() - where
... if ( mdev->p_uuid ) { kfree(mdev->p_uuid); mdev->p_uuid = NULL; } ...
needs to become just
... kfree(mdev->p_uuid); mdev->p_uuid = NULL; ...
And in drbd_receiver.c::drbd_do_auth()
... fail: if(peers_ch) kfree(peers_ch); if(response) kfree(response); if(right_response) kfree(right_response);
...
needs to be turned into
... fail: kfree(peers_ch); kfree(response); kfree(right_response); ...
drbd_req.c::drbd_make_request_common() also needs to get rid of
... if (b) kfree(b); /* if someone else has beaten us to it... */ ... fail_and_free_req: if (b) kfree(b); ...
and just turn it into
... kfree(b); /* if someone else has beaten us to it... */ ... fail_and_free_req: kfree(b); ...
-- Jesper Juhl <jesper.juhl@gmail.com> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html - 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/
| |