Re: [DRIVER SUBMISSION] DRBD wants to go mainline

[Jesper Juhl]

On 21/07/07, Lars Ellenberg <lars@xxxxxxxxxx> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/