Error handling in the ptp2 driver

classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Error handling in the ptp2 driver

Axel Waggershauser
Hello Marcus,

I recently got interested in the new Sony mirror less cameras (A7R and
A6000). Having had a look at the libgphoto code again (I forgot nearly
everything I once knew...) I stumbled over the somewhat inconsistent
error handling code in the ptp2 driver. I'd like to propose a patch
that tries to use a macro similar to the existing CPR and CPR_free
macros wherever applicable to reduce the "noise" in lines like these:

    ret = ptp_generic_getdevicepropdesc (params,
PTP_DPC_CompressionSetting, &dpd);
    if (ret != PTP_RC_OK)
        return translate_ptp_result (ret);

and replace them with something like

    C_PTP( ptp_generic_getdevicepropdesc (params,
PTP_DPC_CompressionSetting, &dpd) );


Before I waste my time though, I have a couple of questions:

1) I'd like to drop the explicit parameter "context" from the macro
and simply use it implicitly. Just like the parameter "params" is
already used. Any objections?

2) How did you decide whether to call report_result (i.e. gp_log the
error and forward the error to the conext->error_func) or only log the
error with gp_log or don't log at all?

3) Why not log pretty much every error passed through translate_ptp_result?

4) Why not also 'report' every error that is logged? Or would it be
plausible to distinct between error conditions that have a 'user
readable/translatable' error message (report them) and those that
don't (only log them with the failed function's name and possibly line
number information)?

5) There is one call of translate_ptp_result in library.c that has no effect:
        ret = ptp_check_eos_events (params);
        if (ret != PTP_RC_OK)
            translate_ptp_result (ret);
There is either a 'return' missing or the the whole check of 'ret' can
be removed. What do you think?

6) Do you object using the __func__ builtin of the preprocessor for an
automatic logging inside a 'CPR'-like macro?

7) If I understand the code, the 'context' is only a set of callback
functions. If there is a non-NULL context at the relevant places, the
respective callback gets called. Why then disable this callback
mechanism by calling SET_CONTEXT_P(..., NULL) in various error
handling paths (but not all)? See for example the
camera_canon_capture() function. I'd strongly favor a way to get rid
of these scattered SET_CONTEXT_P() calls.

8) Why are there two 'ptp_errors' enum-to-string tables, one in
library.c also containing vendor specific error codes and one in
ptp.c? The latter seems never be used by the code, anyway (ptp_perror
never gets called). When consolidating them into one, I'd put that one
in the ptp.c file, next to all the other ptp related enum-to-string
translations.

9) On what compilers is libgphoto2 expected to compile? (Asking with
the ##__VA_ARGS__ extension of gcc in mind.)


Thanks for your feedback,
  Axel

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Marcus Meissner
On Thu, Jun 26, 2014 at 11:14:42AM +0200, Axel Waggershauser wrote:

> Hello Marcus,
>
> I recently got interested in the new Sony mirror less cameras (A7R and
> A6000). Having had a look at the libgphoto code again (I forgot nearly
> everything I once knew...) I stumbled over the somewhat inconsistent
> error handling code in the ptp2 driver. I'd like to propose a patch
> that tries to use a macro similar to the existing CPR and CPR_free
> macros wherever applicable to reduce the "noise" in lines like these:
>
>     ret = ptp_generic_getdevicepropdesc (params,
> PTP_DPC_CompressionSetting, &dpd);
>     if (ret != PTP_RC_OK)
>         return translate_ptp_result (ret);
>
> and replace them with something like
>
>     C_PTP( ptp_generic_getdevicepropdesc (params,
> PTP_DPC_CompressionSetting, &dpd) );
>
>
> Before I waste my time though, I have a couple of questions:
>
> 1) I'd like to drop the explicit parameter "context" from the macro
> and simply use it implicitly. Just like the parameter "params" is
> already used. Any objections?

We can do that, no problem.
 
> 2) How did you decide whether to call report_result (i.e. gp_log the
> error and forward the error to the conext->error_func) or only log the
> error with gp_log or don't log at all?

Not always consciously.

A small rule that needs to be followed:

If the error is not returned to the frontend but handled internally, it must
not put into the context error, as some frontends even report the setting of those
and not just check return values.

> 3) Why not log pretty much every error passed through translate_ptp_result?

We can do that.

> 4) Why not also 'report' every error that is logged? Or would it be
> plausible to distinct between error conditions that have a 'user
> readable/translatable' error message (report them) and those that
> don't (only log them with the failed function's name and possibly line
> number information)?

Frontends will break with superflous context errors. But gp_log
can be called all the time.

> 5) There is one call of translate_ptp_result in library.c that has no effect:
>         ret = ptp_check_eos_events (params);
>         if (ret != PTP_RC_OK)
>             translate_ptp_result (ret);
> There is either a 'return' missing or the the whole check of 'ret' can
> be removed. What do you think?

I forgot it. Fixed in SVN.

> 6) Do you object using the __func__ builtin of the preprocessor for an
> automatic logging inside a 'CPR'-like macro?

Would that work on mingw32? But I think yes. So it would be OK.

> 7) If I understand the code, the 'context' is only a set of callback
> functions. If there is a non-NULL context at the relevant places, the
> respective callback gets called. Why then disable this callback
> mechanism by calling SET_CONTEXT_P(..., NULL) in various error
> handling paths (but not all)? See for example the
> camera_canon_capture() function. I'd strongly favor a way to get rid
> of these scattered SET_CONTEXT_P() calls.

This is not consistently done, yes.

The context backing store in PTPParams need however be consistent on
every function, as an old GPContext might be freed and a new one created
and passed into the next function.

So at least the setting of the context is required, the setting
back to NULL is only for safety reasons.

> 8) Why are there two 'ptp_errors' enum-to-string tables, one in
> library.c also containing vendor specific error codes and one in
> ptp.c? The latter seems never be used by the code, anyway (ptp_perror
> never gets called). When consolidating them into one, I'd put that one
> in the ptp.c file, next to all the other ptp related enum-to-string
> translations.

Sounds good.

> 9) On what compilers is libgphoto2 expected to compile? (Asking with
> the ##__VA_ARGS__ extension of gcc in mind.)

gcc and llvm-clang foremost. People might use Visual C++. although I
am not directly aware of it.

CIao, Marcus

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Marcus Meissner
In reply to this post by Axel Waggershauser
On Thu, Jun 26, 2014 at 11:14:42AM +0200, Axel Waggershauser wrote:
> Hello Marcus,
>
> I recently got interested in the new Sony mirror less cameras (A7R and
> A6000). Having had a look at the libgphoto code again (I forgot nearly
> everything I once knew...) I stumbled over the somewhat inconsistent
> error handling code in the ptp2 driver. I'd like to propose a patch

Sony itself has capture and tethering working, only larger problem
is the configuration handling.

While the values are retrieved nicely (via their configblobs, which works nicely),
the cameras do not seem to allow setting the absolute value, but only
in increments and decrements.

"increase value by 1" and "decrease value by 1".

Check _put_Sony_FNumber for my attempt on this for fnumber.

Ciao, MArcus

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Axel Waggershauser
In reply to this post by Marcus Meissner
Hi Marcus,

I thought I start with an easy part, i.e. the consistent usage of the
old CHECK_PTP_RC() macro in ptp.c (easy - because there is no decision
regarding logging/reporting involved). But one thing let to the next
and I ended up figuring out how the memory management inside the
ptp_transaction_new function works (which I found to be not quite
obvious ;)). I wanted to know how to properly handle an error return
code from ptp_transaction_new with respect to freeing up memory, which
was not done consistently in the various ptp.c functions. I realized
that memory for a receiving ptp transaction gets allocated by
realloc() in memory_putfunc(). A potential failure to allocate this
memory gets handled only in 1 of 4 places inside the calling
ptp_usb_getdata() function in usb.c.

So attached you'll find a patch that cleans up ptp_usb_getdata():
 * remove the code duplication of the 'usbdata.length == 0xffffffffU'
case -> get rid of half of the 'handler->putfunc()' calls
 * consistently check for the return value and the returned number of
'written' bytes of handler->putfunc()
 * replace the 'non-standard' 'do { ... if (error) break; ... } while
(0);' code with the more obvious and standard 'if (error) goto exit;'
idiom, which does the same thing, just more clearly
 * cleanup the confusing bytes-to-be-transferred-related bunch of
variables (removed most of them)
 * change some unsigned long variables to uint32_t, where appropriate

I'd like to suggest to further change the interface of the
PTPDataPutFunc (not in this patch), namely removing the putlen return
parameter. The way it is implemented, only the fd_putfunc may return
anything other than the requested 'sendlen' and the only thing the
calling ptp_usb_getdata can meaningfully do is to check whether
sendlen and putlen are equal and error out if they are not (which was
done in only one of 4 cases). Why not check for the correct number of
written bytes inside fd_putfunc and return an error if appropriate.
This way, the check is at least guaranteed to happen.

I hope you find this ptp_usb_getdata rewrite useful (as I spent more
time on it than I intended ;))

I attached two versions of the patch. The non-ws version is only for
manual inspection: it does not contain the ws-noise from the removal
of the 'do {} while(0)'-loop. I tested the code with my Canon EOS 450D
with a couple of capture-image-and-download tries.

 - Axel

On Thu, Jun 26, 2014 at 11:33 AM, Marcus Meissner <[hidden email]> wrote:

> On Thu, Jun 26, 2014 at 11:14:42AM +0200, Axel Waggershauser wrote:
>> Hello Marcus,
>>
>> I recently got interested in the new Sony mirror less cameras (A7R and
>> A6000). Having had a look at the libgphoto code again (I forgot nearly
>> everything I once knew...) I stumbled over the somewhat inconsistent
>> error handling code in the ptp2 driver. I'd like to propose a patch
>> that tries to use a macro similar to the existing CPR and CPR_free
>> macros wherever applicable to reduce the "noise" in lines like these:
>>
>>     ret = ptp_generic_getdevicepropdesc (params,
>> PTP_DPC_CompressionSetting, &dpd);
>>     if (ret != PTP_RC_OK)
>>         return translate_ptp_result (ret);
>>
>> and replace them with something like
>>
>>     C_PTP( ptp_generic_getdevicepropdesc (params,
>> PTP_DPC_CompressionSetting, &dpd) );
>>
>>
>> Before I waste my time though, I have a couple of questions:
>>
>> 1) I'd like to drop the explicit parameter "context" from the macro
>> and simply use it implicitly. Just like the parameter "params" is
>> already used. Any objections?
>
> We can do that, no problem.
>
>> 2) How did you decide whether to call report_result (i.e. gp_log the
>> error and forward the error to the conext->error_func) or only log the
>> error with gp_log or don't log at all?
>
> Not always consciously.
>
> A small rule that needs to be followed:
>
> If the error is not returned to the frontend but handled internally, it must
> not put into the context error, as some frontends even report the setting of those
> and not just check return values.
>
>> 3) Why not log pretty much every error passed through translate_ptp_result?
>
> We can do that.
>
>> 4) Why not also 'report' every error that is logged? Or would it be
>> plausible to distinct between error conditions that have a 'user
>> readable/translatable' error message (report them) and those that
>> don't (only log them with the failed function's name and possibly line
>> number information)?
>
> Frontends will break with superflous context errors. But gp_log
> can be called all the time.
>
>> 5) There is one call of translate_ptp_result in library.c that has no effect:
>>         ret = ptp_check_eos_events (params);
>>         if (ret != PTP_RC_OK)
>>             translate_ptp_result (ret);
>> There is either a 'return' missing or the the whole check of 'ret' can
>> be removed. What do you think?
>
> I forgot it. Fixed in SVN.
>
>> 6) Do you object using the __func__ builtin of the preprocessor for an
>> automatic logging inside a 'CPR'-like macro?
>
> Would that work on mingw32? But I think yes. So it would be OK.
>
>> 7) If I understand the code, the 'context' is only a set of callback
>> functions. If there is a non-NULL context at the relevant places, the
>> respective callback gets called. Why then disable this callback
>> mechanism by calling SET_CONTEXT_P(..., NULL) in various error
>> handling paths (but not all)? See for example the
>> camera_canon_capture() function. I'd strongly favor a way to get rid
>> of these scattered SET_CONTEXT_P() calls.
>
> This is not consistently done, yes.
>
> The context backing store in PTPParams need however be consistent on
> every function, as an old GPContext might be freed and a new one created
> and passed into the next function.
>
> So at least the setting of the context is required, the setting
> back to NULL is only for safety reasons.
>
>> 8) Why are there two 'ptp_errors' enum-to-string tables, one in
>> library.c also containing vendor specific error codes and one in
>> ptp.c? The latter seems never be used by the code, anyway (ptp_perror
>> never gets called). When consolidating them into one, I'd put that one
>> in the ptp.c file, next to all the other ptp related enum-to-string
>> translations.
>
> Sounds good.
>
>> 9) On what compilers is libgphoto2 expected to compile? (Asking with
>> the ##__VA_ARGS__ extension of gcc in mind.)
>
> gcc and llvm-clang foremost. People might use Visual C++. although I
> am not directly aware of it.
>
> CIao, Marcus

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel

ptp_usb_getdata.patch (17K) Download Attachment
ptp_usb_getdata.patch-non-ws (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Marcus Meissner-4
Hi Axel,

Starting with one of the more fragile pieces ... ;)

I have applied your patch after reviewing.

As it was not fully clear I think:
0xffffffffU size is (also) the PTP size for objects larger than 4GB,
usually movies.

And I originally added some "unsigned long" to allow sizes larger than
32bit. In your code I think only bytes_read might get larger than 32bit,
the rest appears safe and bytes_read is only used in progress context.

(Have to find a device that handles files larger than 4GB for
 testing though :/)

Ciao, Marcus


On Tue, Jul 01, 2014 at 11:27:30PM +0200, Axel Waggershauser wrote:

> Hi Marcus,
>
> I thought I start with an easy part, i.e. the consistent usage of the
> old CHECK_PTP_RC() macro in ptp.c (easy - because there is no decision
> regarding logging/reporting involved). But one thing let to the next
> and I ended up figuring out how the memory management inside the
> ptp_transaction_new function works (which I found to be not quite
> obvious ;)). I wanted to know how to properly handle an error return
> code from ptp_transaction_new with respect to freeing up memory, which
> was not done consistently in the various ptp.c functions. I realized
> that memory for a receiving ptp transaction gets allocated by
> realloc() in memory_putfunc(). A potential failure to allocate this
> memory gets handled only in 1 of 4 places inside the calling
> ptp_usb_getdata() function in usb.c.
>
> So attached you'll find a patch that cleans up ptp_usb_getdata():
>  * remove the code duplication of the 'usbdata.length == 0xffffffffU'
> case -> get rid of half of the 'handler->putfunc()' calls
>  * consistently check for the return value and the returned number of
> 'written' bytes of handler->putfunc()
>  * replace the 'non-standard' 'do { ... if (error) break; ... } while
> (0);' code with the more obvious and standard 'if (error) goto exit;'
> idiom, which does the same thing, just more clearly
>  * cleanup the confusing bytes-to-be-transferred-related bunch of
> variables (removed most of them)
>  * change some unsigned long variables to uint32_t, where appropriate
>
> I'd like to suggest to further change the interface of the
> PTPDataPutFunc (not in this patch), namely removing the putlen return
> parameter. The way it is implemented, only the fd_putfunc may return
> anything other than the requested 'sendlen' and the only thing the
> calling ptp_usb_getdata can meaningfully do is to check whether
> sendlen and putlen are equal and error out if they are not (which was
> done in only one of 4 cases). Why not check for the correct number of
> written bytes inside fd_putfunc and return an error if appropriate.
> This way, the check is at least guaranteed to happen.

Hmm. Hard to say if we want or even can handle short writes or should
just error out on those.

I would not mind this change (and check if anything breaks).

> I hope you find this ptp_usb_getdata rewrite useful (as I spent more
> time on it than I intended ;))

It was. It is more readable now.
 
> I attached two versions of the patch. The non-ws version is only for
> manual inspection: it does not contain the ws-noise from the removal
> of the 'do {} while(0)'-loop. I tested the code with my Canon EOS 450D
> with a couple of capture-image-and-download tries.

I tested iRiver and Creative Zen, they still work good too.

Ciao, Marcus

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Rogier Wolff
On Wed, Jul 02, 2014 at 09:38:58AM +0200, Marcus Meissner wrote:
> And I originally added some "unsigned long" to allow sizes larger than
> 32bit.

Ehh... That won't work. "unsigned long" is 32 bits on a 32-bit
compiler (e.g. ia32), and happens to be 64-bit on 64-bit compilers
(e.g. x84_64).

You'd get nasty: "it doesn't work on my system" bugs that are
difficult to diagnose like this.

To get 64-bit variables and portability between 64-bit and 32-bit
compilers, you need to use "long long" or uint64_t and let the header
files figure out what the compiler wants to make a 64-bit variable.

        Roger.

--
** [hidden email] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
**    Delftechpark 26 2628 XH  Delft, The Netherlands. KVK: 27239233    **
*-- BitWizard writes Linux device drivers for any device you may have! --*
The plan was simple, like my brother-in-law Phil. But unlike
Phil, this plan just might work.

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Marcus Meissner
On Wed, Jul 02, 2014 at 10:38:27AM +0200, Rogier Wolff wrote:

> On Wed, Jul 02, 2014 at 09:38:58AM +0200, Marcus Meissner wrote:
> > And I originally added some "unsigned long" to allow sizes larger than
> > 32bit.
>
> Ehh... That won't work. "unsigned long" is 32 bits on a 32-bit
> compiler (e.g. ia32), and happens to be 64-bit on 64-bit compilers
> (e.g. x84_64).
>
> You'd get nasty: "it doesn't work on my system" bugs that are
> difficult to diagnose like this.
>
> To get 64-bit variables and portability between 64-bit and 32-bit
> compilers, you need to use "long long" or uint64_t and let the header
> files figure out what the compiler wants to make a 64-bit variable.

I am aware that it was half fixed and I was (incorrectly) afraid to use uint64_t

Ciao, Marcus

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Axel Waggershauser
In reply to this post by Marcus Meissner-4
Hi Marcus,

> As it was not fully clear I think:
> 0xffffffffU size is (also) the PTP size for objects larger than 4GB,
> usually movies.

Yes. I interpreted the 0xffffffffU case as "read until a short packet
arrives, which might be until the end of time". But I now saw, that
forgot one change regarding the report_progress behaviour: It makes no
sense to report a progress when we don't know how much more data is
coming. See attached patch.


> Hmm. Hard to say if we want or even can handle short writes or should
> just error out on those.
>
> I would not mind this change (and check if anything breaks).

Very well, see attachment.


>> I hope you find this ptp_usb_getdata rewrite useful (as I spent more
>> time on it than I intended ;))
>
> It was. It is more readable now.

Nice.

On to the next adventure... ;)

 - Axel

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel

fix_report_progress_in_ptp_usb_getdata.patch (804 bytes) Download Attachment
remove_putlen_return_parameter.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Axel Waggershauser
Hi Marcus,

back to my original "easy part": attached is a set of patches focused
on the ptp.c file. It it rather big as it touches all of the functions
connected to ptp_transaction, i.e. most of the ptp_* functions. My
goal expanded from "using CHECK_PTP_RC where appropriate" to "make the
usage of ptp_transaction as consistent as possible / things that do
the same should look the same", which was necessary to even understand
where a simple return of an error code is ok and where some memory
allocated inside ptp_transaction has to be released.

To increase consistency, I

a) renamed variables (e.g. all local 'data' and 'size' parameters
passed on to ptp_transaction are called the same now)
b) removed unnecessary variables wherever possible (like 'ret' or 'size')
c) reordered the declaration of local variables to be consistent
d) tried to unify the error checks and error handling patterns
e) defined ptp_transaction to properly initialize the 'data' and
'reclen' parameters (-> remove external initialization)
f) defined ptp_transaction to free potentially allocated memory if an
error occurred (checked and fixed all 21 usage cases)
g) introduced a neat PTPContainer initialization method, similar to
the ptp_generic_no_data() stuff we introduced a couple of years ago

Part 2 contains only the code from g). Part 3 should actually have
been in Part 1 but I only realized it after Part 2 was done already.

All this fixed at least a handful of leaks where a 'free' was missed
in certain error conditions. I also added a bunch of missing error
checks. In total, the patches reduce the code size about 500 lines.
I'd suggest to remove another 100+ lines by dropping the declaration
of the 'PTPContainer ptp;' and move it inside the updated
PTP_CNT_INIT() macro. It would be a purely mechanical change. I did
not do it, since I wanted to make sure you don't object declaring
variables inside macros.

I also did a couple of white-space changes, just to increase
consistency/readability of the code but this was not the focus. There
could be done a lot more in this regard if one wishes to do so.

I added a couple of TODO's where the error checking as it was/is,
seems faulty or nonexistent.

I realized that the different unpacking/packing functions have very
different error handling (none/return bool/return PTP_RC_*). This
should also be looked into sooner or later.

On Wed, Jul 2, 2014 at 1:22 PM, Axel Waggershauser <[hidden email]> wrote:
>> It was. It is more readable now.

I hope (even more than last time aground) that you'll find this
useful, again. ;)

 - Axel

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel

ptp_c-cleanup-1 (95K) Download Attachment
ptp_c-cleanup-2 (50K) Download Attachment
ptp_c-cleanup-3 (27K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Axel Waggershauser
In reply to this post by Marcus Meissner
Hi Marcus,

On Thu, Jun 26, 2014 at 11:33 AM, Marcus Meissner <[hidden email]> wrote:
>> 1) I'd like to drop the explicit parameter "context" from the macro
>> and simply use it implicitly. Just like the parameter "params" is
>> already used. Any objections?
>
> We can do that, no problem.

Please see attached patch. It does only one thing: drop the explicit
'context' parameter of CPR().

(And sorry for that last 'patch-bomb'. I've been working on this
cleanup for the better part of the week and did not have a clear idea
what it might look like in the end. The next patches are going to be
more 'digestible'. ;))


>> 3) Why not log pretty much every error passed through translate_ptp_result?
>
> We can do that.

I'll look into that next.


>> 7) If I understand the code, the 'context' is only a set of callback
>> functions. If there is a non-NULL context at the relevant places, the
>> respective callback gets called. Why then disable this callback
>> mechanism by calling SET_CONTEXT_P(..., NULL) in various error
>> handling paths (but not all)? See for example the
>> camera_canon_capture() function. I'd strongly favor a way to get rid
>> of these scattered SET_CONTEXT_P() calls.
>
> This is not consistently done, yes.
>
> The context backing store in PTPParams need however be consistent on
> every function, as an old GPContext might be freed and a new one created
> and passed into the next function.
So it would be semantically correct if every function that gets passed
a context parameter and a camera parameter, resets the context value
inside the camera's private data (even though this would be
unnecessary)? And it would be sufficient, if every function directly
called from user-code does that. Those should exactly be the
camera->functions, right?

>> 8) Why are there two 'ptp_errors' enum-to-string tables, one in
>> library.c also containing vendor specific error codes and one in
>> ptp.c? The latter seems never be used by the code, anyway (ptp_perror
>> never gets called). When consolidating them into one, I'd put that one
>> in the ptp.c file, next to all the other ptp related enum-to-string
>> translations.
>
> Sounds good.

See patch. I basically moved the library.c version of ptp_error to
ptp.c and unified the error strings somewhat (consistent casing and
wording).

The drop-context-parameter-of-CPR patch has to come first.

 - Axel

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel

drop-context-parameter-of-CPR.patch (29K) Download Attachment
ptp-error-string-cleanup.patch (22K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Marcus Meissner-4
In reply to this post by Axel Waggershauser
Hi,

Thanks for the patchset!

I have reviewed and applied all 3 patches.

only small adjustments:
- fixed some new olympus warnings (you probably do not have libxml2
  development installed)

- moved some uint16_t ret; declarations in part 3 back to prolog

On Thu, Jul 03, 2014 at 05:24:26PM +0200, Axel Waggershauser wrote:

> Hi Marcus,
>
> All this fixed at least a handful of leaks where a 'free' was missed
> in certain error conditions. I also added a bunch of missing error
> checks. In total, the patches reduce the code size about 500 lines.
> I'd suggest to remove another 100+ lines by dropping the declaration
> of the 'PTPContainer ptp;' and move it inside the updated
> PTP_CNT_INIT() macro. It would be a purely mechanical change. I did
> not do it, since I wanted to make sure you don't object declaring
> variables inside macros.

I would prefer not having it inside the macro.

> I also did a couple of white-space changes, just to increase
> consistency/readability of the code but this was not the focus. There
> could be done a lot more in this regard if one wishes to do so.

The diff showed some new tab/whitespace mixups added, but this can be done
at some later point.

> I added a couple of TODO's where the error checking as it was/is,
> seems faulty or nonexistent.
>
> I realized that the different unpacking/packing functions have very
> different error handling (none/return bool/return PTP_RC_*). This
> should also be looked into sooner or later.

Yes.

Ciao, Marcus

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Axel Waggershauser
> Thanks for the patchset!
>
> I have reviewed and applied all 3 patches.

Thanks. But it seems the 3rd is not applied/committed, yet?

> only small adjustments:
> - fixed some new olympus warnings (you probably do not have libxml2
>   development installed)

That is correct.

> - moved some uint16_t ret; declarations in part 3 back to prolog

I see. I'll try to remember your preference here for the future.


> I would prefer not having it inside the macro.

Very well. I agree that this change would actually decrease readability.

 - A

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Marcus Meissner
On Fri, Jul 04, 2014 at 11:48:23AM +0200, Axel Waggershauser wrote:
> > Thanks for the patchset!
> >
> > I have reviewed and applied all 3 patches.
>
> Thanks. But it seems the 3rd is not applied/committed, yet?

Huh. I might have forgotten the svn ci at home. But it will be ;)
 

> > only small adjustments:
> > - fixed some new olympus warnings (you probably do not have libxml2
> >   development installed)
>
> That is correct.
>
> > - moved some uint16_t ret; declarations in part 3 back to prolog
>
> I see. I'll try to remember your preference here for the future.
>
>
> > I would prefer not having it inside the macro.
>
> Very well. I agree that this change would actually decrease readability.

Ciao, Marcus

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Marcus Meissner-4
In reply to this post by Axel Waggershauser
On Thu, Jul 03, 2014 at 08:04:11PM +0200, Axel Waggershauser wrote:

> Hi Marcus,
>
> On Thu, Jun 26, 2014 at 11:33 AM, Marcus Meissner <[hidden email]> wrote:
> >> 1) I'd like to drop the explicit parameter "context" from the macro
> >> and simply use it implicitly. Just like the parameter "params" is
> >> already used. Any objections?
> >
> > We can do that, no problem.
>
> Please see attached patch. It does only one thing: drop the explicit
> 'context' parameter of CPR().
>
> (And sorry for that last 'patch-bomb'. I've been working on this
> cleanup for the better part of the week and did not have a clear idea
> what it might look like in the end. The next patches are going to be
> more 'digestible'. ;))
 
Okay :) Took a bit to read over, but not that long.

I will run my camera tests this weekend and see if anything broke. ;)

 

> > The context backing store in PTPParams need however be consistent on
> > every function, as an old GPContext might be freed and a new one created
> > and passed into the next function.
>
> So it would be semantically correct if every function that gets passed
> a context parameter and a camera parameter, resets the context value
> inside the camera's private data (even though this would be
> unnecessary)? And it would be sufficient, if every function directly
> called from user-code does that. Those should exactly be the
> camera->functions, right?

Yes, the toplevel functions set in camera->functions should have them
and that should be sufficient.

 

> >> 8) Why are there two 'ptp_errors' enum-to-string tables, one in
> >> library.c also containing vendor specific error codes and one in
> >> ptp.c? The latter seems never be used by the code, anyway (ptp_perror
> >> never gets called). When consolidating them into one, I'd put that one
> >> in the ptp.c file, next to all the other ptp related enum-to-string
> >> translations.
> >
> > Sounds good.
>
> See patch. I basically moved the library.c version of ptp_error to
> ptp.c and unified the error strings somewhat (consistent casing and
> wording).
>
> The drop-context-parameter-of-CPR patch has to come first.

Both are good, applied as-is. Thanks!

Ciao, Marcus

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Axel Waggershauser
Hi Marcus,

I finally got to where I started out with looking into the error
handling: using some 'CR'-like macro to reduce noise in the
source-code. Attached is a 5-patch series:

1) use the CPR macro where appropriate (check and report ptp errors)
2) use the CR macro where appropriate (check and report gp errors)
3) rename CPR to C_PTP_REP (check ptp and report/translate) + add
logging to the new C_PTP_REP
4) add logging to CR
5) introduce 3 more macros similar to C_PTP_REP:
  * C_PTP: check ptp-errors, log them with gp_log and translate them
to gp-errors
  * C_PTP_MSG: same as C_PTP but with a custom printf-like error message string
  * C_PTP_REP_MSG: same as C_PTP_REP but with a custom printf-like
error message string

The *_REP_* versions are there to distinguish between errors that
should only be logged and those that should also be reported to the
front-end. It might make sense to reevaluate which errors should be
reported and which should not. I did not change the behaviour here.

The *_MSG versions are there, because I could not find a way to have a
"default" empty message string parameter for the macro. The usage is

    C_PTP (ptp_some_func(x));
    C_PTP_MSG (ptp_some_func(x), "could not set %d", x);

and this would produce a logging line similar to this:

    'ptp_some_func(x)' failed: 'PTP Parameter Not Supported' (0x2006)
    'ptp_some_func(x)' failed: 'could not set 5' (0x2006: 'PTP
Parameter Not Supported')

If one would be happy with writing C_PTP(ptp_some_func(x), ""); all
the time, then the two versions could be merged to one. I preferred
having the extra _MSG version, though.

All C_PTP* macros log a numerical and textual representation of the
PTP_RC_* error code at hand and also log both the calling function and
the called function.

Please take a serious look at the gettext related stuff in the
C_PTP_REP_MSG macro. I tried to make sure that also the combined error
string (custom error message plus ptp_error string) get properly
translated. I did not test it though.

I dropped the old 'custom' error message in case where it was only
logged (not reported) and only contained a hint to the failing
function, which gets logged automatically with C_PTP now.

There are probably a lot of places where the return codes of ptp_* and
gp_* functions are ignored simply because they were overlooked or it
was to cumbersome to add the 'manual' check. Those should be checked
with C_PTP/CR now. But I don't have the insight nor the testing
capabilities to decide which errors should rightfully be ignored. I
added very few C_PTP calls where it seemed obvious (like some EOS-code
that I have written back then).

The combined patches remove about another 500 lines of code.

Cheers,
  Axel

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel

err-cleanup-1-use-existing-CPR-where-possible.patch (9K) Download Attachment
err-cleanup-2-use-existing-CR-where-possible.patch (35K) Download Attachment
err-cleanup-3-CPR-to-C_PTP_REP.patch (29K) Download Attachment
err-cleanup-4-add-logging-to-CR.patch (890 bytes) Download Attachment
err-cleanup-5-add-C_PTP-macros.patch (88K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Marcus Meissner-4
Hi,

I applied the 5 patches after review.

Will do some testing after the weekend.

> If one would be happy with writing C_PTP(ptp_some_func(x), ""); all
> the time, then the two versions could be merged to one. I preferred
> having the extra _MSG version, though.

Yeah, that would be kind of ugly.
 
> Please take a serious look at the gettext related stuff in the
> C_PTP_REP_MSG macro. I tried to make sure that also the combined error
> string (custom error message plus ptp_error string) get properly
> translated. I did not test it though.

Looks generally good.

> I dropped the old 'custom' error message in case where it was only
> logged (not reported) and only contained a hint to the failing
> function, which gets logged automatically with C_PTP now.
>
> There are probably a lot of places where the return codes of ptp_* and
> gp_* functions are ignored simply because they were overlooked or it
> was to cumbersome to add the 'manual' check. Those should be checked
> with C_PTP/CR now. But I don't have the insight nor the testing
> capabilities to decide which errors should rightfully be ignored. I
> added very few C_PTP calls where it seemed obvious (like some EOS-code
> that I have written back then).

Some of them are ignored because they might also fail. Lets see if
something weird shows up during testing.

Ciao, Marcus

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Axel Waggershauser
While working on the logging code (see other thread) I read through a
lot of other, non-ptp2 code and came to the conclusion that a
unification of the different naming schemes in the error checking
macros would be helpful, too. I found the following ones:

* CR, CHECK, CHECK_RESULT: check and return on GP_ERRORs
* CHECK_NULL: checks for not-null and returns GP_ERROR_BAD_PARAMETERS
* CHECK_MEM: checks for not-null and returns GP_ERROR_NO_MEMORY
* CHECK_AND_FREE checks and returns on GP_ERRORs + free's memory
* C_PTP_*: check for ptp-error codes and translate to GP_ERRORs

and also a bunch of more special purpose ones:

* CHECK_LIST
* CHECK_INIT
* CHECK_SUPP
* CHECK_OPEN
* CHECK_CLOSE
* CHECK_PTP_RC
* CHECK_INDEX_RANGE
* CU
* CC
* CL
* CA
* CRS
* CRSL

I would also like to add other similar "special purpose" macros,
specifically one for checking and translating libusb1 errors. Before
making it all even more confusing I'd like to suggest a common
'theme'. The question is what would be best?

Obviously the long versions (CHECK_*) are more easy to reason about
but more typing is involved. I don't like the very short 2 to 3 letter
ones. With the recent ptp-stuff I introduced the C_PTP_* ones, just
because CHECK_PTP_REP_MSG seemed awfully long.

Here is my suggestion, request for comment:

a) unify the naming with a common prefix, either 'C_' or 'CHK'_ or
'CHECK_'. What would be your preference, Marcus?

b) unify CR, CHECK, CHECK_RESULT to e.g. C_GP to give some hint at the
functions they apply to, namely gp_* function (returning GP_ERROR_*
codes). This should help a bit with not accidentally writing something
like 'CHECK (ptp_some_func())', which I've come across recently.

c) rename the *_NULL macro to *_PARAMS to make sure one does not use
it for malloc checking

d) place the general purpose ones in gphoto-port-log.h to be
accessible from everywhere

e) add automatic logging in case of an error, just because it is for free

 - Axel

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Axel Waggershauser
Appendix: I missed a couple more: CRF, C, CRW, C_FREE, CHECK_FREE,
CHECK_free. Which should only contribute to my argument ;).

On Fri, Jul 11, 2014 at 2:37 PM, Axel Waggershauser <[hidden email]> wrote:

> While working on the logging code (see other thread) I read through a
> lot of other, non-ptp2 code and came to the conclusion that a
> unification of the different naming schemes in the error checking
> macros would be helpful, too. I found the following ones:
>
> * CR, CHECK, CHECK_RESULT: check and return on GP_ERRORs
> * CHECK_NULL: checks for not-null and returns GP_ERROR_BAD_PARAMETERS
> * CHECK_MEM: checks for not-null and returns GP_ERROR_NO_MEMORY
> * CHECK_AND_FREE checks and returns on GP_ERRORs + free's memory
> * C_PTP_*: check for ptp-error codes and translate to GP_ERRORs
>
> and also a bunch of more special purpose ones:
>
> * CHECK_LIST
> * CHECK_INIT
> * CHECK_SUPP
> * CHECK_OPEN
> * CHECK_CLOSE
> * CHECK_PTP_RC
> * CHECK_INDEX_RANGE
> * CU
> * CC
> * CL
> * CA
> * CRS
> * CRSL
>
> I would also like to add other similar "special purpose" macros,
> specifically one for checking and translating libusb1 errors. Before
> making it all even more confusing I'd like to suggest a common
> 'theme'. The question is what would be best?
>
> Obviously the long versions (CHECK_*) are more easy to reason about
> but more typing is involved. I don't like the very short 2 to 3 letter
> ones. With the recent ptp-stuff I introduced the C_PTP_* ones, just
> because CHECK_PTP_REP_MSG seemed awfully long.
>
> Here is my suggestion, request for comment:
>
> a) unify the naming with a common prefix, either 'C_' or 'CHK'_ or
> 'CHECK_'. What would be your preference, Marcus?
>
> b) unify CR, CHECK, CHECK_RESULT to e.g. C_GP to give some hint at the
> functions they apply to, namely gp_* function (returning GP_ERROR_*
> codes). This should help a bit with not accidentally writing something
> like 'CHECK (ptp_some_func())', which I've come across recently.
>
> c) rename the *_NULL macro to *_PARAMS to make sure one does not use
> it for malloc checking
>
> d) place the general purpose ones in gphoto-port-log.h to be
> accessible from everywhere
>
> e) add automatic logging in case of an error, just because it is for free
>
>  - Axel

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Axel Waggershauser
In reply to this post by Axel Waggershauser
Hi Marcus,

I have not heard your opinion regarding this suggestion, therefore
this 'bump' e-mail. What do you think about the following suggestions?


> a) unify the naming with a common prefix, either 'C_' or 'CHK'_ or
> 'CHECK_'. What would be your preference, Marcus?
>
> b) unify CR, CHECK, CHECK_RESULT to e.g. C_GP to give some hint at the
> functions they apply to, namely gp_* function (returning GP_ERROR_*
> codes). This should help a bit with not accidentally writing something
> like 'CHECK (ptp_some_func())', which I've come across recently.
>
> c) rename the *_NULL macro to *_PARAMS to make sure one does not use
> it for malloc checking
>
> d) place the general purpose ones in gphoto-port-log.h to be
> accessible from everywhere
I 'ported' the libusb1.c file to this approach, including the addition
of a 'special-purpose' LIBUSB-reated check macro to deal with the
fact, that some gp_port_... functions returned GP_ERROR_IO_READ others
GP_ERROR_IO_WRITE regardless of what the underlying libusb_...
function returned. The 'generic' check macros C_GP, C_MEM and C_PARAM
would later be moved into gphoto-port-log.h as suggested.

I was quite happy with the suggested macros in the 'default' cases,
where the macro logs the error and returns an appropriate error code.
But sometimes simply returning/exiting the function is not enough. I
found a couple of re-occurring non-default cases that don't allow the
basic macro based check. Those are:

* a 'goto' statement to jump to some cleanup part of the function
(like freeing memory or the like)
* returning a special value (like -1) instead of the usual error code
* a 'continue' statement inside a while/for loop
* nothing at all (simply logging the error, nothing else)
* calling free() to cleanup memory
* using another error reporting mechanism, like gp_port_set_error

In libusb1.c where about 10 default and 20 non-default cases with
respect to libusb_... calls. Leaving the non-default cases to the old
and verbose if-err-then-log-and-do-somehting pattern bugged me. Adding
tons of macros similar to CHECK_FREE() for each non-default case is
ugly, too. I found a neat generalization of the macro idea to even
capture all those non-default cases:

// macro with custom error handling statement for all cases
#define C_LIBUS_AND_DO(RESULT, ...) {\
    int _r = (RESULT);\
    if (_r != LIBUSB_SUCCESS) {\
        GP_LOG_E ("'%s' failed: %s (0x%x)", #RESULT, libusb_strerror(_r), _r);\
        __VA_ARGS__\
    }\
}

// macro with default error handling statement
#define C_LIBUSB(RESULT, DEFAULT_ERROR)\
    C_LIBUSB_AND_DO(RESULT,\
        return translate_libusb_error(_r, DEFAULT_ERROR);\
    )

This covers all usage patterns:

    C_LIBUSB (libusb_func1()); // default case

    C_LIBUSB_AND_DO (libusb_func2()); // don't return, just log error and go on

    C_LIBUSB_AND_DO (libusb_func3(),
        gp_port_set_error (port, _("some text")); // special error reporting
        return GP_SOME_ERROR; // could also be goto, break or continue
statements
    );

Instead of extending the suggested naming C_LIBUSB with
C_LIBUSB_AND_DO, two other reasonable names came to my mind:
ON_LIBUSB_ERR_DO and IF_LIBUSB_ERR_THEN. They better show that there
is basically an if-statement behind but they look very different
compared to the basic C_... version. Any preference? I can easily
rename the macro and send an updated patch.

The attached patches also add logging to all libusb_... calls where
there has not been any (about half). It fixes a nasty bug, where the
LIBUSB_ERROR_... code gets simply returned as a GP_ERROR_... code. It
adds one missing libusb_... error handling. It does translate standard
errors like bad_param or out_of_memory to the respective GP_ERROR code
instead of returning GP_ERROR_IO everywhere.

Cheers,
  Axel

------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel

libusb1-cleanup-1-check-param.patch (9K) Download Attachment
libusb1-cleanup-2-check-libusb.patch (27K) Download Attachment
libusb1-cleanup-3-check-gp.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Error handling in the ptp2 driver

Marcus Meissner
In reply to this post by Marcus Meissner-4
On Fri, Jul 11, 2014 at 08:56:54AM +0200, Marcus Meissner wrote:
> Hi,
>
> I applied the 5 patches after review.
>
> Will do some testing after the weekend.

Some feedback here.

- PTP Image download was fully broken, so all PTP users were broken.

  (due to a shadowing variable "ret" in the download code). Fixed.

- A potentially latent Canon powershot event handling bug was exposed (probably
  my fault). Also fixed.

Nikon D7000, EOS 100D, and PowerShot SX100IS tests scripts ran successful
afterwards.

Ciao, Marcus

------------------------------------------------------------------------------
Want fast and easy access to all the code in your enterprise? Index and
search up to 200,000 lines of code with a free copy of Black Duck
Code Sight - the same software that powers the world's largest code
search on Ohloh, the Black Duck Open Hub! Try it now.
http://p.sf.net/sfu/bds
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
12