Re: [PATCH 2/2] uemis downloader: start downloading using the correct dive IDavoid NULL pointer dereference

[Guido Lerch]

Tested this today. Works with my 240 dives I have by now on the Uemis.

Also the blindly strstr was obvious and an overlook on my side.

The initial object id wasn't from me :-)

So I am happy with my newbie contributions :-)

The code to update selected dives is ready but requires a UI change. I suppose you don't want this for 4.5.

G. Lerch


Sonntag, 20. September 2015 06:14 +0200 von Linus Torvalds  <torvalds at linux-foundation.org>:
>
>From: Linus Torvalds < torvalds at linux-foundation.org >
>Date: Sat, 19 Sep 2015 21:09:58 -0700
>Subject: [PATCH 2/2] uemis downloader: start downloading using the correct dive ID
>
>The logic to pick the initial dive ID for the uemis downloader was very
>confused, and did not work at all when restarting a download when the
>Uemis filled up, and the "Force download all dives" flag was set.  It
>also required a rather odd Uemis-specific callback from the download UI
>because of how it picked the initial ID.
>
>This changes the logic to just look at the list of downloaded dives when
>restarting, which simplifies the logic a lot, gets rid of the odd
>special callback, and also means that the whole "Force download" issue
>just goes away.  It seems to work now.
>
>Signed-off-by: Linus Torvalds < torvalds at linux-foundation.org >
>---
> dive.h                             |  2 +-
> qt-ui/downloadfromdivecomputer.cpp |  8 --------
> uemis-downloader.c                 | 40 +++++++++++++++++++++-----------------
> uemis.h                            |  1 -
> 4 files changed, 23 insertions(+), 28 deletions(-)
>
>diff --git a/dive.h b/dive.h
>index fa95f6721631..4eb44cfc9c8d 100644
>--- a/dive.h
>+++ b/dive.h
>@@ -498,7 +498,7 @@ struct dive_table {
> 	struct dive **dives;
> };
> 
>-extern struct dive_table dive_table;
>+extern struct dive_table dive_table, downloadTable;
> extern struct dive displayed_dive;
> extern struct dive_site displayed_dive_site;
> extern int selected_dive;
>diff --git a/qt-ui/downloadfromdivecomputer.cpp b/qt-ui/downloadfromdivecomputer.cpp
>index 7e18dae70baa..1fef9f6bff1e 100644
>--- a/qt-ui/downloadfromdivecomputer.cpp
>+++ b/qt-ui/downloadfromdivecomputer.cpp
>@@ -298,14 +298,6 @@ void DownloadFromDCWidget::on_downloadCancelRetryButton_clicked()
> 		diveImportedModel->clearTable();
> 		clear_table(&downloadTable);
> 	}
>-	if (ui.vendor->currentText() == "Uemis") {
>-		if (currentState == ERROR && downloadTable.nr > 0)
>-			// let the uemis code know how far we've gotten
>-			uemis_set_max_diveid_from_dialog(downloadTable.dives[downloadTable.nr - 1]->dc.diveid);
>-		else
>-			// fresh download, so only look at what's in the dive_table
>-			uemis_set_max_diveid_from_dialog(0);
>-	}
> 	updateState(DOWNLOADING);
> 
> 	// you cannot cancel the dialog, just the download
>diff --git a/uemis-downloader.c b/uemis-downloader.c
>index 70cc3f78d449..3b8ddf900e89 100644
>--- a/uemis-downloader.c
>+++ b/uemis-downloader.c
>@@ -928,23 +928,32 @@ static bool process_raw_buffer(device_data_t *devdata, uint32_t deviceid, char *
> 	return true;
> }
> 
>-static int max_diveid_from_dialog;
>-
>-void uemis_set_max_diveid_from_dialog(int diveid)
>-{
>-	max_diveid_from_dialog = diveid;
>-}
>-
>-static char *uemis_get_divenr(char *deviceidstr)
>+static char *uemis_get_divenr(char *deviceidstr, int force)
> {
> 	uint32_t deviceid, maxdiveid = 0;
> 	int i;
> 	char divenr[10];
>-
>+	struct dive_table *table;
> 	deviceid = atoi(deviceidstr);
>-	struct dive *d;
>-	for_each_dive (i, d) {
>+
>+	/*
>+	 * If we are are retrying after a disconnect/reconnect, we
>+	 * will look up the highest dive number in the dives we
>+	 * already have.
>+	 *
>+	 * Also, if "force_download" is true, do this even if we
>+	 * don't have any dives (maxdiveid will remain zero)
>+	 */
>+	if (force || downloadTable.nr)
>+		table = &downloadTable;
>+	else
>+		table = &dive_table;
>+
>+	for (i = 0; i < table->nr; i++) {
>+		struct dive *d = table->dives[i];
> 		struct divecomputer *dc;
>+		if (!d)
>+			continue;
> 		for_each_dc (d, dc) {
> 			if (dc->model && !strcmp(dc->model, "Uemis Zurich") &&
> 			    (dc->deviceid == 0 || dc->deviceid == 0x7fffffff || dc->deviceid == deviceid) &&
>@@ -952,7 +961,7 @@ static char *uemis_get_divenr(char *deviceidstr)
> 				maxdiveid = dc->diveid;
> 		}
> 	}
>-	snprintf(divenr, 10, "%d", maxdiveid > max_diveid_from_dialog ? maxdiveid : max_diveid_from_dialog);
>+	snprintf(divenr, 10, "%d", maxdiveid);
> 	return strdup(divenr);
> }
> 
>@@ -1215,12 +1224,7 @@ const char *do_uemis_import(device_data_t *data)
> 		goto bail;
> 
> 	param_buff[1] = "notempty";
>-	/* if we force it we start downloading from the first dive on the Uemis;
>-	 *  otherwise check which was the last dive downloaded */
>-	if (!force_download)
>-		newmax = uemis_get_divenr(deviceid);
>-	else
>-		newmax = strdup("0");
>+	newmax = uemis_get_divenr(deviceid, force_download);
> 
> 	first = start = atoi(newmax);
> 	dive_to_read = first;
>diff --git a/uemis.h b/uemis.h
>index 90ae99028799..5f32fe76c0af 100644
>--- a/uemis.h
>+++ b/uemis.h
>@@ -16,7 +16,6 @@ void uemis_parse_divelog_binary(char *base64, void *divep);
> int uemis_get_weight_unit(int diveid);
> void uemis_mark_divelocation(int diveid, int divespot, uint32_t dive_site_uuid);
> void uemis_set_divelocation(int divespot, char *text, double longitude, double latitude);
>-void uemis_set_max_diveid_from_dialog(int diveid);
> int uemis_get_divespot_id_by_diveid(uint32_t diveid);
> 
> typedef struct
>-- 
>2.6.0.rc1.16.g1962994
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150921/df43cc87/attachment-0001.html>