OSTCTools import support

[Dirk Hohndel]

Hi Salva,

On Sun, Mar 29, 2015 at 08:46:29PM +0200, Salvador Cuñat wrote:
> From 686d24da47aa36692d905ed84d9f3aec2abd21ea Mon Sep 17 00:00:00 2001
> Actually dive_cb() needs a defined device to work as its built for DC
> download tasks.
> Move part of the functionality of dive_cb() to a new function
> libdc_header_parser() which can be used to parse headers from raw data
> buffers with no device set.
> Obviously dive_cb() will call the new funtion for header parsing too.

Hard to read patch... some comments below

> diff --git a/libdivecomputer.c b/libdivecomputer.c
> index 950b9da..8b461ac 100644
> --- a/libdivecomputer.c
> +++ b/libdivecomputer.c
> @@ -400,72 +400,50 @@ static void parse_string_field(struct dive *dive, dc_field_string_t *str)
>  }
>  #endif
>  
> -/* returns true if we want libdivecomputer's dc_device_foreach() to continue,
> - *  false otherwise */
> -static int dive_cb(const unsigned char *data, unsigned int size,
> -		   const unsigned char *fingerprint, unsigned int fsize,
> -		   void *userdata)
> +static dc_status_t libdc_header_parser(dc_parser_t *parser, struct device_data_t *devdata, struct dive *dive)
>  {
> -	int rc;
> -	dc_parser_t *parser = NULL;
> -	device_data_t *devdata = userdata;
> -	dc_datetime_t dt = { 0 };
> +	dc_status_t rc = 0;
> +	dc_datetime_t datetime = { 0 };

so you renamed dt to datetime - nothing wrong with that, but it makes the
patch more verbose and harder to see what's new, what's just renaming...

> -	rc = dc_parser_get_datetime(parser, &dt);
> +	rc = dc_parser_get_datetime(parser, &datetime);

like this.

>  	if (rc != DC_STATUS_SUCCESS && rc != DC_STATUS_UNSUPPORTED) {
>  		dev_info(devdata, translate("gettextFromC", "Error parsing the datetime"));
> -		goto error_exit;
> +		fprintf(stderr, "Error parsing the datetime");
> +		return rc;

fprintf to stderr is reasonable for debugging or for fatal errors,
otherwise it's not ideal. Are these errors that should be reported to the
user? Or really just debugging messages?

>  	if (rc == DC_STATUS_SUCCESS) {
> -		tm.tm_year = dt.year;
> -		tm.tm_mon = dt.month - 1;
> -		tm.tm_mday = dt.day;
> -		tm.tm_hour = dt.hour;
> -		tm.tm_min = dt.minute;
> -		tm.tm_sec = dt.second;
> +		tm.tm_year = datetime.year;
> +		tm.tm_mon = datetime.month - 1;
> +		tm.tm_mday = datetime.day;
> +		tm.tm_hour = datetime.hour;
> +		tm.tm_min = datetime.minute;
> +		tm.tm_sec = datetime.second;

See, this is what I mean. That rename in the middle of the patch REALLY
adds to the noise and hides the other changes...

>  	if (rc != DC_STATUS_SUCCESS && rc != DC_STATUS_UNSUPPORTED) {
>  		dev_info(devdata, translate("gettextFromC", "Error parsing the divetime"));
> -		goto error_exit;
> +		fprintf(stderr, "Error parsing the divetime\n");
> +		return rc;

Same here and in several more places. The fprintf just isn't a good fit, I
think

> -		dive->dc.duration.seconds = divetime;
> +		dive->duration.seconds = dive->dc.duration.seconds = divetime;

Why? fixup_dive() should be called on any imported dive at some point and
that calls fixup_duration() which handles this consistently.

> @@ -537,7 +519,7 @@ static int dive_cb(const unsigned char *data, unsigned int size,
>  	for (idx = 0; idx < 100; idx++) {
>  		dc_field_string_t str = { NULL };
>  		rc = dc_parser_get_field(parser, DC_FIELD_STRING, idx, &str);
> -		if (rc != DC_STATUS_SUCCESS)
> +		if (rc != DC_STATUS_SUCCESS && rc != DC_STATUS_UNSUPPORTED)

Can you explain this change?

> @@ -558,21 +541,67 @@ static int dive_cb(const unsigned char *data, unsigned int size,
>  			dive->dc.divemode = FREEDIVE;
>  			break;
>  		case DC_DIVEMODE_GAUGE:
> -		case DC_DIVEMODE_OC: /* Open circuit */
> +		case DC_DIVEMODE_OC:
>  			dive->dc.divemode = OC;
>  			break;
> -		case DC_DIVEMODE_CC:  /* Closed circuit */
> +		case DC_DIVEMODE_CC:

And why would you remove those comments?

>  #endif
>  
> -	rc = parse_gasmixes(devdata, dive, parser, ngases, data);
> -	if (rc != DC_STATUS_SUCCESS) {
> +	rc = parse_gasmixes(devdata, dive, parser, ngases, NULL);
> +	if (rc != DC_STATUS_SUCCESS && rc != DC_STATUS_UNSUPPORTED) {

Maybe I got lost reading the patch - but why are you passing NULL instead
of data?

>  		dev_info(devdata, translate("gettextFromC", "Error parsing the gas mix"));
> +		fprintf(stderr, "Error parsing the gas mix\n");

Umm - so there is the correct error reporting to the user, and you add an
fprintf to stderr. Not good.


Here's what I'd like to see. Either don't do the datetime rename or if you
do it, have it in its own patch with nothing else mixed in.
Then, if you want to change all the error returns to show these errors to
the user, do that in one patch and nothing else.
As a third patch you can do the split out into a separate function,
explaining any logic changes that you are doing.

Thanks

/D