Patchwork [v2] Bluetooth: hidp: using strlcpy instead of strncpy, also beautify code.

login
register
mail settings
Submitter Chen Gang
Date May 8, 2013, 3:34 a.m.
Message ID <5189C7C6.8090408@asianux.com>
Download mbox | patch
Permalink /patch/242494/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Chen Gang - May 8, 2013, 3:34 a.m.
For NUL terminated string, need always let it ended by zero.

Since have already called memcpy() to initialize 'ci', so need not
redundent initializations.

Better use ''if(session->hid) {} else if(session->input) {}'' instead
of ''if(session->hid) {}; if(session->input) {};''

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 net/bluetooth/hidp/core.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)
David Herrmann - May 8, 2013, 3:16 p.m.
Hi Chen

On Wed, May 8, 2013 at 5:34 AM, Chen Gang <gang.chen@asianux.com> wrote:
>
> For NUL terminated string, need always let it ended by zero.
>
> Since have already called memcpy() to initialize 'ci', so need not
> redundent initializations.
>
> Better use ''if(session->hid) {} else if(session->input) {}'' instead
> of ''if(session->hid) {}; if(session->input) {};''

Yep, looks good now.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  net/bluetooth/hidp/core.c |   14 ++++----------
>  1 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 940f5ac..f13a8da 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -76,25 +76,19 @@ static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo
>         ci->flags = session->flags;
>         ci->state = BT_CONNECTED;
>
> -       ci->vendor  = 0x0000;
> -       ci->product = 0x0000;
> -       ci->version = 0x0000;
> -
>         if (session->input) {
>                 ci->vendor  = session->input->id.vendor;
>                 ci->product = session->input->id.product;
>                 ci->version = session->input->id.version;
>                 if (session->input->name)
> -                       strncpy(ci->name, session->input->name, 128);
> +                       strlcpy(ci->name, session->input->name, 128);
>                 else
> -                       strncpy(ci->name, "HID Boot Device", 128);
> -       }
> -
> -       if (session->hid) {
> +                       strlcpy(ci->name, "HID Boot Device", 128);
> +       } else if (session->hid) {
>                 ci->vendor  = session->hid->vendor;
>                 ci->product = session->hid->product;
>                 ci->version = session->hid->version;
> -               strncpy(ci->name, session->hid->name, 128);
> +               strlcpy(ci->name, session->hid->name, 128);
>         }
>  }
>
> --
> 1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen Gang - May 9, 2013, 1:07 a.m.
On 05/08/2013 11:16 PM, David Herrmann wrote:
> On Wed, May 8, 2013 at 5:34 AM, Chen Gang <gang.chen@asianux.com> wrote:
>> >
>> > For NUL terminated string, need always let it ended by zero.
>> >
>> > Since have already called memcpy() to initialize 'ci', so need not
>> > redundent initializations.
>> >
>> > Better use ''if(session->hid) {} else if(session->input) {}'' instead
>> > of ''if(session->hid) {}; if(session->input) {};''
> Yep, looks good now.
> 
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks.
Jiri Kosina - May 9, 2013, 8:42 a.m.
On Wed, 8 May 2013, Chen Gang wrote:

> 
> For NUL terminated string, need always let it ended by zero.
> 
> Since have already called memcpy() to initialize 'ci', so need not
> redundent initializations.
> 
> Better use ''if(session->hid) {} else if(session->input) {}'' instead
> of ''if(session->hid) {}; if(session->input) {};''
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>

Makes sense.

Acked-by: Jiri Kosina <jkosina@suse.cz>

Gustavo, going to take this, please?
Chen Gang - May 9, 2013, 8:48 a.m.
On 05/09/2013 04:42 PM, Jiri Kosina wrote:
> On Wed, 8 May 2013, Chen Gang wrote:
> 
>> > 
>> > For NUL terminated string, need always let it ended by zero.
>> > 
>> > Since have already called memcpy() to initialize 'ci', so need not
>> > redundent initializations.
>> > 
>> > Better use ''if(session->hid) {} else if(session->input) {}'' instead
>> > of ''if(session->hid) {}; if(session->input) {};''
>> > 
>> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
> Makes sense.
> 
> Acked-by: Jiri Kosina <jkosina@suse.cz>
> 
> Gustavo, going to take this, please?
> 
> -- Jiri Kosina SUSE Labs
> 

Thanks, and also help to fix the spelling: redundent -> redundant.
(if need let me send patch v3, please reply to tell me, thanks)

Patch

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 940f5ac..f13a8da 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -76,25 +76,19 @@  static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo
 	ci->flags = session->flags;
 	ci->state = BT_CONNECTED;
 
-	ci->vendor  = 0x0000;
-	ci->product = 0x0000;
-	ci->version = 0x0000;
-
 	if (session->input) {
 		ci->vendor  = session->input->id.vendor;
 		ci->product = session->input->id.product;
 		ci->version = session->input->id.version;
 		if (session->input->name)
-			strncpy(ci->name, session->input->name, 128);
+			strlcpy(ci->name, session->input->name, 128);
 		else
-			strncpy(ci->name, "HID Boot Device", 128);
-	}
-
-	if (session->hid) {
+			strlcpy(ci->name, "HID Boot Device", 128);
+	} else if (session->hid) {
 		ci->vendor  = session->hid->vendor;
 		ci->product = session->hid->product;
 		ci->version = session->hid->version;
-		strncpy(ci->name, session->hid->name, 128);
+		strlcpy(ci->name, session->hid->name, 128);
 	}
 }