[Pkg-running-devel] Bug#816314: Bug#816314: Fix patch for libusb1.0 update.

[Fenix]

     Hi. I comment beween quotes. :)



On 17/04/16 09:26, Christian PERRIER wrote:


>> @@ -97,22 +96,28 @@ garmin_open ( garmin_unit * garmin )
>>   	  if ( err ) {
>>   	    printf("libusb_open failed: %s\n",libusb_error_name(err));
>>               garmin->usb.handle = NULL;
>> -	  } else if ( garmin->verbose != 0 ) {
>> -	    printf("[garmin] libusb_open = %p\n",garmin->usb.handle);
>> +	  } else {
>> +	      if ( garmin->verbose != 0 ) {
>> +                printf("[garmin] libusb_open = %p\n",garmin->usb.handle);
>> +	      }
>
> I'm not really skilled in C, but isn't that just cosmetic?


    No, it isn't just cosmetic. The problem is that structure is a bit 
ugly (I agree). Original makes:


     if (error) {
         [...]
     } else if (only_execute_when_user_wants_verbosity) {
          print message
          err = execute_core_function_libusb

          if (err) {
               [...]
	 } else if (only_execute_when_user_wants_verbosity) {
               [...]
          }
     }


     In that situation, the function (execute_core_function_libusb) of 
libusb library only are executed when the user runs the program with -v 
main parameter (wants verbosity).

     You could safe the {} of the if (verbosity] condition as it is just 
contain one sentence. Make something like:


    if (error) {
    } else {
        if (verbosity) print message
        err = execute_core_function_libusb

        if (err) {
            [...]
        } else {
          if (verbosity) print message
	 err = execute_core_function_libusb
        }
    }


    But I prefer mark the block of code with {} for readbility, 
specially in that situation.

    I'm not skilled C programmer, too (I used loooong time ago :)), but 
I probably didn't make that structure. I'd just make:


    If (error) {
       [...]
    } else {
       if (verbosity) print message
       err = execute_core_function_1_libusb
    }

    If (not error) {
      if (verbosity) print message
      err = execute_core_function__n_libusb
    }


    Probably is better for readbility, but it is less efficient. So, I 
preferred to maintain the original structure and make a "quick fix".



>
>>
>> -            err = libusb_set_configuration(garmin->usb.handle,1);
>> +              err = libusb_set_configuration(garmin->usb.handle,1);
>
> Ditto
>
>>               if ( err ) {
>> -              printf("libusb_set_configuration failed: %s\n",
>> +                printf("libusb_set_configuration failed: %s\n",
>
> Ditto
>
> And so on....
>
> I'd be happy to apply the patch, of course, but do you think that it
> can be "cleaned"?
>
> I can try to do it myself but......I'm a bit afraid to break things
> doing so.
>
>


   I agree. Of course can be cleaned. :)

   I can clean the non-functional cosmetic (unintentional from me) 
changes of the patch: lines, blanks...


   Maybe you want to add to this conversation the person who makes the 
libusb1.0 port as I'm not skilled with libusb (only learned the needed 
to make this fix). As I said, the fix works for me and the tests I made 
goes fine, but maybe there is something I did not consider.



   I'll try to make the re-patch along today.



   Thanks. :)