[Qemu-devel] [PATCH] libcacard: remove useless initializers

[Qemu-devel] [PATCH] libcacard: remove useless initializers

[Michael Tokarev]

libcacard has many functions which initializes local variables
at declaration time, which are always assigned some values later
(often right after declaration).  Clean up these initializers.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 libcacard/cac.c            |   14 +++++++-------
 libcacard/card_7816.c      |    5 ++---
 libcacard/vcard.c          |    4 ++--
 libcacard/vcard_emul_nss.c |    6 +++---
 libcacard/vreader.c        |   10 +++++-----
 libcacard/vscclient.c      |    4 ++--
 6 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/libcacard/cac.c b/libcacard/cac.c
index 122129e..d1d9ee2 100644
--- a/libcacard/cac.c
+++ b/libcacard/cac.c
@@ -93,8 +93,8 @@ cac_common_process_apdu(VCard *card, VCardAPDU *apdu, VCardResponse **response)
 static VCardStatus
 cac_applet_pki_reset(VCard *card, int channel)
 {
-    VCardAppletPrivate *applet_private = NULL;
-    CACPKIAppletData *pki_applet = NULL;
+    VCardAppletPrivate *applet_private;
+    CACPKIAppletData *pki_applet;
     applet_private = vcard_get_current_applet_private(card, channel);
     assert(applet_private);
     pki_applet = &(applet_private->u.pki_data);
@@ -113,8 +113,8 @@ static VCardStatus
 cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
                             VCardResponse **response)
 {
-    CACPKIAppletData *pki_applet = NULL;
-    VCardAppletPrivate *applet_private = NULL;
+    CACPKIAppletData *pki_applet;
+    VCardAppletPrivate *applet_private;
     int size, next;
     unsigned char *sign_buffer;
     vcard_7816_status_t status;
@@ -288,7 +288,7 @@ cac_applet_container_process_apdu(VCard *card, VCardAPDU *apdu,
 static void
 cac_delete_pki_applet_private(VCardAppletPrivate *applet_private)
 {
-    CACPKIAppletData *pki_applet_data = NULL;
+    CACPKIAppletData *pki_applet_data;
 
     if (applet_private == NULL) {
         return;
@@ -336,8 +336,8 @@ static VCardApplet *
 cac_new_pki_applet(int i, const unsigned char *cert,
                    int cert_len, VCardKey *key)
 {
-    VCardAppletPrivate *applet_private = NULL;
-    VCardApplet *applet = NULL;
+    VCardAppletPrivate *applet_private;
+    VCardApplet *applet;
     unsigned char pki_aid[] = { 0xa0, 0x00, 0x00, 0x00, 0x79, 0x01, 0x00 };
     int pki_aid_len = sizeof(pki_aid);
 
diff --git a/libcacard/card_7816.c b/libcacard/card_7816.c
index bca8c4a..a54f880 100644
--- a/libcacard/card_7816.c
+++ b/libcacard/card_7816.c
@@ -416,7 +416,7 @@ VCARD_RESPONSE_NEW_STATIC_STATUS(VCARD7816_STATUS_ERROR_GENERAL)
 VCardResponse *
 vcard_make_response(vcard_7816_status_t status)
 {
-    VCardResponse *response = NULL;
+    VCardResponse *response;
 
     switch (status) {
     /* known 7816 response codes */
@@ -543,9 +543,8 @@ vcard_make_response(vcard_7816_status_t status)
             return VCARD_RESPONSE_GET_STATIC(
                         VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE);
         }
+        return response;
     }
-    assert(response);
-    return response;
 }
 
 /*
diff --git a/libcacard/vcard.c b/libcacard/vcard.c
index 227e477..6aaf085 100644
--- a/libcacard/vcard.c
+++ b/libcacard/vcard.c
@@ -166,8 +166,8 @@ vcard_reference(VCard *vcard)
 void
 vcard_free(VCard *vcard)
 {
-    VCardApplet *current_applet = NULL;
-    VCardApplet *next_applet = NULL;
+    VCardApplet *current_applet;
+    VCardApplet *next_applet;
 
     if (vcard == NULL) {
         return;
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 75b9d79..3f38a4c 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -367,7 +367,7 @@ vcard_7816_status_t
 vcard_emul_login(VCard *card, unsigned char *pin, int pin_len)
 {
     PK11SlotInfo *slot;
-    unsigned char *pin_string = NULL;
+    unsigned char *pin_string;
     int i;
     SECStatus rv;
 
@@ -423,7 +423,7 @@ static VReader *
 vcard_emul_find_vreader_from_slot(PK11SlotInfo *slot)
 {
     VReaderList *reader_list = vreader_get_reader_list();
-    VReaderListEntry *current_entry = NULL;
+    VReaderListEntry *current_entry;
 
     if (reader_list == NULL) {
         return NULL;
@@ -1050,7 +1050,7 @@ void
 vcard_emul_replay_insertion_events(void)
 {
     VReaderListEntry *current_entry;
-    VReaderListEntry *next_entry = NULL;
+    VReaderListEntry *next_entry;
     VReaderList *list = vreader_get_reader_list();
 
     for (current_entry = vreader_list_get_first(list); current_entry;
diff --git a/libcacard/vreader.c b/libcacard/vreader.c
index 9304a28..22dfe43 100644
--- a/libcacard/vreader.c
+++ b/libcacard/vreader.c
@@ -341,7 +341,7 @@ void
 vreader_list_delete(VReaderList *list)
 {
     VReaderListEntry *current_entry;
-    VReaderListEntry *next_entry = NULL;
+    VReaderListEntry *next_entry;
     for (current_entry = vreader_list_get_first(list); current_entry;
          current_entry = next_entry) {
         next_entry = vreader_list_get_next(current_entry);
@@ -432,8 +432,8 @@ vreader_list_unlock(void)
 static VReaderList *
 vreader_copy_list(VReaderList *list)
 {
-    VReaderList *new_list = NULL;
-    VReaderListEntry *current_entry = NULL;
+    VReaderList *new_list;
+    VReaderListEntry *current_entry;
 
     new_list = vreader_list_new();
     if (new_list == NULL) {
@@ -465,7 +465,7 @@ VReader *
 vreader_get_reader_by_id(vreader_id_t id)
 {
     VReader *reader = NULL;
-    VReaderListEntry *current_entry = NULL;
+    VReaderListEntry *current_entry;
 
     if (id == (vreader_id_t) -1) {
         return NULL;
@@ -489,7 +489,7 @@ VReader *
 vreader_get_reader_by_name(const char *name)
 {
     VReader *reader = NULL;
-    VReaderListEntry *current_entry = NULL;
+    VReaderListEntry *current_entry;
 
     vreader_list_lock();
     for (current_entry = vreader_list_get_first(vreader_list); current_entry;
diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
index 3477ab3..17ff075 100644
--- a/libcacard/vscclient.c
+++ b/libcacard/vscclient.c
@@ -131,8 +131,8 @@ static void *
 event_thread(void *arg)
 {
     unsigned char atr[MAX_ATR_LEN];
-    int atr_len = MAX_ATR_LEN;
-    VEvent *event = NULL;
+    int atr_len;
+    VEvent *event;
     unsigned int reader_id;
 
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] libcacard: remove useless initializers

[Alon Levy]

On 05/08/2014 08:19 PM, Michael Tokarev wrote:
> libcacard has many functions which initializes local variables
> at declaration time, which are always assigned some values later
> (often right after declaration).  Clean up these initializers.

How is this an improvement? Doesn't the compiler ignore this anyhow?

> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  libcacard/cac.c            |   14 +++++++-------
>  libcacard/card_7816.c      |    5 ++---
>  libcacard/vcard.c          |    4 ++--
>  libcacard/vcard_emul_nss.c |    6 +++---
>  libcacard/vreader.c        |   10 +++++-----
>  libcacard/vscclient.c      |    4 ++--
>  6 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/libcacard/cac.c b/libcacard/cac.c
> index 122129e..d1d9ee2 100644
> --- a/libcacard/cac.c
> +++ b/libcacard/cac.c
> @@ -93,8 +93,8 @@ cac_common_process_apdu(VCard *card, VCardAPDU *apdu, VCardResponse **response)
>  static VCardStatus
>  cac_applet_pki_reset(VCard *card, int channel)
>  {
> -    VCardAppletPrivate *applet_private = NULL;
> -    CACPKIAppletData *pki_applet = NULL;
> +    VCardAppletPrivate *applet_private;
> +    CACPKIAppletData *pki_applet;
>      applet_private = vcard_get_current_applet_private(card, channel);
>      assert(applet_private);
>      pki_applet = &(applet_private->u.pki_data);
> @@ -113,8 +113,8 @@ static VCardStatus
>  cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
>                              VCardResponse **response)
>  {
> -    CACPKIAppletData *pki_applet = NULL;
> -    VCardAppletPrivate *applet_private = NULL;
> +    CACPKIAppletData *pki_applet;
> +    VCardAppletPrivate *applet_private;
>      int size, next;
>      unsigned char *sign_buffer;
>      vcard_7816_status_t status;
> @@ -288,7 +288,7 @@ cac_applet_container_process_apdu(VCard *card, VCardAPDU *apdu,
>  static void
>  cac_delete_pki_applet_private(VCardAppletPrivate *applet_private)
>  {
> -    CACPKIAppletData *pki_applet_data = NULL;
> +    CACPKIAppletData *pki_applet_data;
>  
>      if (applet_private == NULL) {
>          return;
> @@ -336,8 +336,8 @@ static VCardApplet *
>  cac_new_pki_applet(int i, const unsigned char *cert,
>                     int cert_len, VCardKey *key)
>  {
> -    VCardAppletPrivate *applet_private = NULL;
> -    VCardApplet *applet = NULL;
> +    VCardAppletPrivate *applet_private;
> +    VCardApplet *applet;
>      unsigned char pki_aid[] = { 0xa0, 0x00, 0x00, 0x00, 0x79, 0x01, 0x00 };
>      int pki_aid_len = sizeof(pki_aid);
>  
> diff --git a/libcacard/card_7816.c b/libcacard/card_7816.c
> index bca8c4a..a54f880 100644
> --- a/libcacard/card_7816.c
> +++ b/libcacard/card_7816.c
> @@ -416,7 +416,7 @@ VCARD_RESPONSE_NEW_STATIC_STATUS(VCARD7816_STATUS_ERROR_GENERAL)
>  VCardResponse *
>  vcard_make_response(vcard_7816_status_t status)
>  {
> -    VCardResponse *response = NULL;
> +    VCardResponse *response;
>  
>      switch (status) {
>      /* known 7816 response codes */
> @@ -543,9 +543,8 @@ vcard_make_response(vcard_7816_status_t status)
>              return VCARD_RESPONSE_GET_STATIC(
>                          VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE);
>          }
> +        return response;
>      }
> -    assert(response);
> -    return response;
>  }
>  
>  /*
> diff --git a/libcacard/vcard.c b/libcacard/vcard.c
> index 227e477..6aaf085 100644
> --- a/libcacard/vcard.c
> +++ b/libcacard/vcard.c
> @@ -166,8 +166,8 @@ vcard_reference(VCard *vcard)
>  void
>  vcard_free(VCard *vcard)
>  {
> -    VCardApplet *current_applet = NULL;
> -    VCardApplet *next_applet = NULL;
> +    VCardApplet *current_applet;
> +    VCardApplet *next_applet;
>  
>      if (vcard == NULL) {
>          return;
> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
> index 75b9d79..3f38a4c 100644
> --- a/libcacard/vcard_emul_nss.c
> +++ b/libcacard/vcard_emul_nss.c
> @@ -367,7 +367,7 @@ vcard_7816_status_t
>  vcard_emul_login(VCard *card, unsigned char *pin, int pin_len)
>  {
>      PK11SlotInfo *slot;
> -    unsigned char *pin_string = NULL;
> +    unsigned char *pin_string;
>      int i;
>      SECStatus rv;
>  
> @@ -423,7 +423,7 @@ static VReader *
>  vcard_emul_find_vreader_from_slot(PK11SlotInfo *slot)
>  {
>      VReaderList *reader_list = vreader_get_reader_list();
> -    VReaderListEntry *current_entry = NULL;
> +    VReaderListEntry *current_entry;
>  
>      if (reader_list == NULL) {
>          return NULL;
> @@ -1050,7 +1050,7 @@ void
>  vcard_emul_replay_insertion_events(void)
>  {
>      VReaderListEntry *current_entry;
> -    VReaderListEntry *next_entry = NULL;
> +    VReaderListEntry *next_entry;
>      VReaderList *list = vreader_get_reader_list();
>  
>      for (current_entry = vreader_list_get_first(list); current_entry;
> diff --git a/libcacard/vreader.c b/libcacard/vreader.c
> index 9304a28..22dfe43 100644
> --- a/libcacard/vreader.c
> +++ b/libcacard/vreader.c
> @@ -341,7 +341,7 @@ void
>  vreader_list_delete(VReaderList *list)
>  {
>      VReaderListEntry *current_entry;
> -    VReaderListEntry *next_entry = NULL;
> +    VReaderListEntry *next_entry;
>      for (current_entry = vreader_list_get_first(list); current_entry;
>           current_entry = next_entry) {
>          next_entry = vreader_list_get_next(current_entry);
> @@ -432,8 +432,8 @@ vreader_list_unlock(void)
>  static VReaderList *
>  vreader_copy_list(VReaderList *list)
>  {
> -    VReaderList *new_list = NULL;
> -    VReaderListEntry *current_entry = NULL;
> +    VReaderList *new_list;
> +    VReaderListEntry *current_entry;
>  
>      new_list = vreader_list_new();
>      if (new_list == NULL) {
> @@ -465,7 +465,7 @@ VReader *
>  vreader_get_reader_by_id(vreader_id_t id)
>  {
>      VReader *reader = NULL;
> -    VReaderListEntry *current_entry = NULL;
> +    VReaderListEntry *current_entry;
>  
>      if (id == (vreader_id_t) -1) {
>          return NULL;
> @@ -489,7 +489,7 @@ VReader *
>  vreader_get_reader_by_name(const char *name)
>  {
>      VReader *reader = NULL;
> -    VReaderListEntry *current_entry = NULL;
> +    VReaderListEntry *current_entry;
>  
>      vreader_list_lock();
>      for (current_entry = vreader_list_get_first(vreader_list); current_entry;
> diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c
> index 3477ab3..17ff075 100644
> --- a/libcacard/vscclient.c
> +++ b/libcacard/vscclient.c
> @@ -131,8 +131,8 @@ static void *
>  event_thread(void *arg)
>  {
>      unsigned char atr[MAX_ATR_LEN];
> -    int atr_len = MAX_ATR_LEN;
> -    VEvent *event = NULL;
> +    int atr_len;
> +    VEvent *event;
>      unsigned int reader_id;
>  
>  
> 

Re: [Qemu-devel] [PATCH] libcacard: remove useless initializers

[Michael Tokarev]

11.05.2014 11:58, Alon Levy wrote:
> On 05/08/2014 08:19 PM, Michael Tokarev wrote:
>> libcacard has many functions which initializes local variables
>> at declaration time, which are always assigned some values later
>> (often right after declaration).  Clean up these initializers.
> 
> How is this an improvement? Doesn't the compiler ignore this anyhow?

Just less code.

To me, when I see something like

  Type *var = NULL;

in a function, it somehow "translates" to a construct like

  Type *found = NULL;

That is -- so this variable will be used either as an accumulator
or a search result, so that initial value is really important.

So when I see the same variable receives its initial value in
the next line, I start wondering what's missed in the code which
should be there.  Or why I don't read the code correctly.  Or
something like this.

So, basically, this is a cleanup patch just to avoid confusion,
it most likely not needed for current compiler who can figure
it out by its own.  And for consistency - why not initialize
other variables too?

Maybe that's just my old-scool mind works this way.

At any rate you can just ignore this patch.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] libcacard: remove useless initializers

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 11.05.2014 11:58, Alon Levy wrote:
>> On 05/08/2014 08:19 PM, Michael Tokarev wrote:
>>> libcacard has many functions which initializes local variables
>>> at declaration time, which are always assigned some values later
>>> (often right after declaration).  Clean up these initializers.
>> 
>> How is this an improvement? Doesn't the compiler ignore this anyhow?
>
> Just less code.
>
> To me, when I see something like
>
>   Type *var = NULL;
>
> in a function, it somehow "translates" to a construct like
>
>   Type *found = NULL;
>
> That is -- so this variable will be used either as an accumulator
> or a search result, so that initial value is really important.
>
> So when I see the same variable receives its initial value in
> the next line, I start wondering what's missed in the code which
> should be there.  Or why I don't read the code correctly.  Or
> something like this.
>
> So, basically, this is a cleanup patch just to avoid confusion,
> it most likely not needed for current compiler who can figure
> it out by its own.  And for consistency - why not initialize
> other variables too?

I hate redundant initializers for yet another reason: when I change the
code, and accidentally add a path bypassing the *real* initialization, I
don't get a "may be used uninitialized" warning, I get the stupid
redundant initialization and quite possibly a crash to debug some time
later.

> Maybe that's just my old-scool mind works this way.
>
> At any rate you can just ignore this patch.

Please consider it.

Re: [Qemu-devel] [Qemu-trivial] [PATCH] libcacard: remove useless initializers

[Michael Tokarev]

So, should we apply this or not?  It's been waiting for quite some time,
and during this time we've found a very good example of why it should
be applied (I think anyway).

Thanks,

/mjt


12.05.2014 13:20, Markus Armbruster wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> 11.05.2014 11:58, Alon Levy wrote:
>>> On 05/08/2014 08:19 PM, Michael Tokarev wrote:
>>>> libcacard has many functions which initializes local variables
>>>> at declaration time, which are always assigned some values later
>>>> (often right after declaration).  Clean up these initializers.
>>>
>>> How is this an improvement? Doesn't the compiler ignore this anyhow?
>>
>> Just less code.
>>
>> To me, when I see something like
>>
>>   Type *var = NULL;
>>
>> in a function, it somehow "translates" to a construct like
>>
>>   Type *found = NULL;
>>
>> That is -- so this variable will be used either as an accumulator
>> or a search result, so that initial value is really important.
>>
>> So when I see the same variable receives its initial value in
>> the next line, I start wondering what's missed in the code which
>> should be there.  Or why I don't read the code correctly.  Or
>> something like this.
>>
>> So, basically, this is a cleanup patch just to avoid confusion,
>> it most likely not needed for current compiler who can figure
>> it out by its own.  And for consistency - why not initialize
>> other variables too?
> 
> I hate redundant initializers for yet another reason: when I change the
> code, and accidentally add a path bypassing the *real* initialization, I
> don't get a "may be used uninitialized" warning, I get the stupid
> redundant initialization and quite possibly a crash to debug some time
> later.
> 
>> Maybe that's just my old-scool mind works this way.
>>
>> At any rate you can just ignore this patch.
> 
> Please consider it.
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH] libcacard: remove useless initializers

[Alon Levy]

On 05/23/2014 11:59 PM, Michael Tokarev wrote:
> So, should we apply this or not?  It's been waiting for quite some time,
> and during this time we've found a very good example of why it should
> be applied (I think anyway).

I'm fine with applying it, I changed my mind.

> 
> Thanks,
> 
> /mjt
> 
> 
> 12.05.2014 13:20, Markus Armbruster wrote:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
>>
>>> 11.05.2014 11:58, Alon Levy wrote:
>>>> On 05/08/2014 08:19 PM, Michael Tokarev wrote:
>>>>> libcacard has many functions which initializes local variables
>>>>> at declaration time, which are always assigned some values later
>>>>> (often right after declaration).  Clean up these initializers.
>>>>
>>>> How is this an improvement? Doesn't the compiler ignore this anyhow?
>>>
>>> Just less code.
>>>
>>> To me, when I see something like
>>>
>>>   Type *var = NULL;
>>>
>>> in a function, it somehow "translates" to a construct like
>>>
>>>   Type *found = NULL;
>>>
>>> That is -- so this variable will be used either as an accumulator
>>> or a search result, so that initial value is really important.
>>>
>>> So when I see the same variable receives its initial value in
>>> the next line, I start wondering what's missed in the code which
>>> should be there.  Or why I don't read the code correctly.  Or
>>> something like this.
>>>
>>> So, basically, this is a cleanup patch just to avoid confusion,
>>> it most likely not needed for current compiler who can figure
>>> it out by its own.  And for consistency - why not initialize
>>> other variables too?
>>
>> I hate redundant initializers for yet another reason: when I change the
>> code, and accidentally add a path bypassing the *real* initialization, I
>> don't get a "may be used uninitialized" warning, I get the stupid
>> redundant initialization and quite possibly a crash to debug some time
>> later.
>>
>>> Maybe that's just my old-scool mind works this way.
>>>
>>> At any rate you can just ignore this patch.
>>
>> Please consider it.
>>
> 
>