Skip to content

RNMT-4515 Fixes an issue with recent devices running Android 11 #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 26, 2020
Merged

RNMT-4515 Fixes an issue with recent devices running Android 11 #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6a19159
feat: add a complex type for the SQL handle
luissilvaos Nov 25, 2020
5d4f4a5
feat: add a java class to match the complex type for the SQL handle
luissilvaos Nov 25, 2020
a69fa8f
feat: update JNI to use the complex type for the SQL handle
luissilvaos Nov 25, 2020
e52eca7
chore: update sqlite-amalgamation to version 3.26.0
luissilvaos Nov 25, 2020
3cf482d
build: update target platform and architectures
luissilvaos Nov 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions java/io/liteglue/SQLiteNative.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ public class SQLiteNative {
public static native long sqlc_db_last_insert_rowid(long db);

/** Interface to C language function: <br> <code> sqlc_handle_t sqlc_db_open(const char * filename, int flags); </code> */
public static native long sqlc_db_open(String filename, int flags);
public static native SQLiteResponse sqlc_db_open(String filename, int flags);

/** Interface to C language function: <br> <code> sqlc_handle_t sqlc_db_prepare_st(sqlc_handle_t db, const char * sql); </code> */
public static native long sqlc_db_prepare_st(long db, String sql);
public static native SQLiteResponse sqlc_db_prepare_st(long db, String sql);

/** Interface to C language function: <br> <code> int sqlc_db_total_changes(sqlc_handle_t db); </code> */
public static native int sqlc_db_total_changes(long db);
Expand Down
20 changes: 20 additions & 0 deletions java/io/liteglue/SQLiteResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package io.liteglue;

public class SQLiteResponse {
private int result;
private long handle;

public SQLiteResponse(int result, long handle) {
this.result = result;
this.handle = handle;
}

public int getResult() {
return this.result;
}

public long getHandle() {
return this.handle;
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline at the end

4 changes: 2 additions & 2 deletions jni/Application.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#APP_ABI := all
APP_ABI := armeabi armeabi-v7a x86 x86_64 arm64-v8a
APP_ABI := armeabi-v7a x86 x86_64 arm64-v8a
# For future consideration (needs testing):
#APP_ABI += mips mips64

APP_PLATFORM := android-23
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we gaining anything from this bump ? Also, reading the docs https://developer.android.com/ndk/guides/application_mk#app_platform it states that this should match the minSdkVersion , 23 is Android 6 but we support Android 5.1 , right ? If there's no gain from this being set as we, I don't see a reason for this to be break change by bumping this to 23

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we only support Android 6+ in MABS 7. Both 5 and 5.1 were dropped.

Copy link

@Chuckytuh Chuckytuh Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're just introducing a breaking change without actually needing it on this package, right? What if tomorrow we need to fix something here that's needs to be applied also on MABS 5, we'll have to deal with release branches and whatnot for no reason

Copy link
Author

@luissilvaos luissilvaos Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your point, but to be honest, I don't see it as a problem. Here are my thoughts regarding this topic:

  • We already have breaking changes in the cordova-sqlite-storage plugin for MABS 7.
  • The current version used by MABS 6 has been a stable version.
  • If we somehow have to deal with some fixes in the lib versions that support cordova-sqlite-storage for MABS 6 we can just create a branch for that.
  • Also, the changes introduced in this PR aren't a simple fix that we can simply port to MABS 6. IMHO, they should only be applied to the version that will be released with MABS 7, due to the eventual impacts that they may cause.

51 changes: 36 additions & 15 deletions native/SQLiteNative_JNI.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@

/* Java->C glue code:
* Java package: io.liteglue.SQLiteNative
* Java method: long sqlc_api_db_open(int sqlc_api_version, java.lang.String filename, int flags)
* C function: sqlc_handle_t sqlc_api_db_open(int sqlc_api_version, const char * filename, int flags);
* Java method: SQLiteResponse sqlc_api_db_open(int sqlc_api_version, java.lang.String filename, int flags)
* C function: sqlc_handle_ct* sqlc_api_db_open(int sqlc_api_version, const char * filename, int flags);
*/
JNIEXPORT jlong JNICALL
JNIEXPORT jobject JNICALL
Java_io_liteglue_SQLiteNative_sqlc_1api_1db_1open__ILjava_lang_String_2I(JNIEnv *env, jclass _unused, jint sqlc_api_version, jstring filename, jint flags) {
const char* _strchars_filename = NULL;
sqlc_handle_t _res;
sqlc_handle_ct* _res;
if ( NULL != filename ) {
_strchars_filename = (*env)->GetStringUTFChars(env, filename, (jboolean*)NULL);
if ( NULL == _strchars_filename ) {
Expand All @@ -27,7 +27,14 @@ Java_io_liteglue_SQLiteNative_sqlc_1api_1db_1open__ILjava_lang_String_2I(JNIEnv
if ( NULL != filename ) {
(*env)->ReleaseStringUTFChars(env, filename, _strchars_filename);
}
return _res;

jclass class = (*env)->FindClass(env,"io/liteglue/SQLiteResponse");
jmethodID constructor = (*env)->GetMethodID(env, class, "<init>", "(IJ)V");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"(IJ)V"? What is this? Is it really a static thing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the convention for the method signature. Check this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will never again complain about JavaScript 😂

jobject instance = (*env)->NewObject(env, class, constructor, _res->result, _res->handle);

free(_res);

return instance;
}


Expand Down Expand Up @@ -124,13 +131,13 @@ Java_io_liteglue_SQLiteNative_sqlc_1db_1last_1insert_1rowid__J(JNIEnv *env, jcla

/* Java->C glue code:
* Java package: io.liteglue.SQLiteNative
* Java method: long sqlc_db_open(java.lang.String filename, int flags)
* C function: sqlc_handle_t sqlc_db_open(const char * filename, int flags);
* Java method: SQLiteResponse sqlc_db_open(java.lang.String filename, int flags)
* C function: sqlc_handle_ct* sqlc_db_open(const char * filename, int flags);
*/
JNIEXPORT jlong JNICALL
JNIEXPORT jobject JNICALL
Java_io_liteglue_SQLiteNative_sqlc_1db_1open__Ljava_lang_String_2I(JNIEnv *env, jclass _unused, jstring filename, jint flags) {
const char* _strchars_filename = NULL;
sqlc_handle_t _res;
sqlc_handle_ct* _res;
if ( NULL != filename ) {
_strchars_filename = (*env)->GetStringUTFChars(env, filename, (jboolean*)NULL);
if ( NULL == _strchars_filename ) {
Expand All @@ -143,19 +150,26 @@ Java_io_liteglue_SQLiteNative_sqlc_1db_1open__Ljava_lang_String_2I(JNIEnv *env,
if ( NULL != filename ) {
(*env)->ReleaseStringUTFChars(env, filename, _strchars_filename);
}
return _res;

jclass class = (*env)->FindClass(env,"io/liteglue/SQLiteResponse");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can refactor this code to avoid duplication?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that will be a good improvement.

jmethodID constructor = (*env)->GetMethodID(env, class, "<init>", "(IJ)V");
jobject instance = (*env)->NewObject(env, class, constructor, _res->result, _res->handle);

free(_res);

return instance;
}


/* Java->C glue code:
* Java package: io.liteglue.SQLiteNative
* Java method: long sqlc_db_prepare_st(long db, java.lang.String sql)
* C function: sqlc_handle_t sqlc_db_prepare_st(sqlc_handle_t db, const char * sql);
* Java method: SQLiteResponse sqlc_db_prepare_st(long db, java.lang.String sql)
* C function: sqlc_handle_ct* sqlc_db_prepare_st(sqlc_handle_t db, const char * sql);
*/
JNIEXPORT jlong JNICALL
JNIEXPORT jobject JNICALL
Java_io_liteglue_SQLiteNative_sqlc_1db_1prepare_1st__JLjava_lang_String_2(JNIEnv *env, jclass _unused, jlong db, jstring sql) {
const char* _strchars_sql = NULL;
sqlc_handle_t _res;
sqlc_handle_ct* _res;
if ( NULL != sql ) {
_strchars_sql = (*env)->GetStringUTFChars(env, sql, (jboolean*)NULL);
if ( NULL == _strchars_sql ) {
Expand All @@ -168,7 +182,14 @@ Java_io_liteglue_SQLiteNative_sqlc_1db_1prepare_1st__JLjava_lang_String_2(JNIEnv
if ( NULL != sql ) {
(*env)->ReleaseStringUTFChars(env, sql, _strchars_sql);
}
return _res;

jclass class = (*env)->FindClass(env,"io/liteglue/SQLiteResponse");
jmethodID constructor = (*env)->GetMethodID(env, class, "<init>", "(IJ)V");
jobject instance = (*env)->NewObject(env, class, constructor, _res->result, _res->handle);

free(_res);

return instance;
}


Expand Down
33 changes: 23 additions & 10 deletions native/sqlc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,32 +6,36 @@

#include "sqlite3.h"

#define BASE_HANDLE_OFFSET 0x100000000LL

#ifdef SQLC_KEEP_ANDROID_LOG
// ref: http://www.ibm.com/developerworks/opensource/tutorials/os-androidndk/index.html
#define MYLOG(...) __android_log_print(ANDROID_LOG_VERBOSE, "sqlc", __VA_ARGS__)
#else
#define MYLOG(...) ;
#endif

#define HANDLE_FROM_VP(p) ( BASE_HANDLE_OFFSET + ( (unsigned char *)(p) - (unsigned char *)NULL ) )
#define HANDLE_TO_VP(h) (void *)( (unsigned char *)NULL + (ptrdiff_t)((h) - BASE_HANDLE_OFFSET) )
#define HANDLE_FROM_VP(p) (( (unsigned char *)(p) - (unsigned char *)NULL ) )
#define HANDLE_TO_VP(h) (void *)( (unsigned char *)NULL + (ptrdiff_t)((h)) )
Comment on lines +16 to +17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this no longer is required. Or is there something to it ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the BASE_OFFSET and left it there to be an helper for the conversions pointer -> long and long -> pointer, and to minimize the changes in the code as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's no longer required because it's no longer +number = pointer -number = error.


int sqlc_api_version_check(int sqlc_api_version)
{
return (sqlc_api_version != SQLC_API_VERSION) ? SQLC_RESULT_ERROR : SQLC_RESULT_OK;
}

sqlc_handle_t sqlc_api_db_open(int sqlc_api_version, const char *filename, int flags)
sqlc_handle_ct* sqlc_api_db_open(int sqlc_api_version, const char *filename, int flags)
{
if (sqlc_api_version != SQLC_API_VERSION) return SQLC_RESULT_ERROR;
if (sqlc_api_version != SQLC_API_VERSION) {
sqlc_handle_ct* resp = malloc(sizeof(sqlc_handle_ct));
resp->result = SQLC_RESULT_ERROR;
resp->handle = 0;
return resp;
}

return sqlc_db_open(filename, flags);
}

sqlc_handle_t sqlc_db_open(const char *filename, int flags)
sqlc_handle_ct* sqlc_db_open(const char *filename, int flags)
{
sqlc_handle_ct *resp;
sqlite3 *d1;
int r1;

Expand All @@ -41,11 +45,16 @@ sqlc_handle_t sqlc_db_open(const char *filename, int flags)

MYLOG("db_open %s result %d ptr %p", filename, r1, d1);

return (r1 == 0) ? HANDLE_FROM_VP(d1) : -r1;
resp = malloc (sizeof (sqlc_handle_ct));
resp->result = (r1 == 0) ? 0 : -r1;
resp->handle = HANDLE_FROM_VP(d1);

return resp;
}

sqlc_handle_t sqlc_db_prepare_st(sqlc_handle_t db, const char *sql)
sqlc_handle_ct* sqlc_db_prepare_st(sqlc_handle_t db, const char *sql)
{
sqlc_handle_ct *resp;
sqlite3 *mydb = HANDLE_TO_VP(db);
sqlite3_stmt *s;
int rv;
Expand All @@ -54,7 +63,11 @@ sqlc_handle_t sqlc_db_prepare_st(sqlc_handle_t db, const char *sql)

rv = sqlite3_prepare_v2(mydb, sql, -1, &s, NULL);

return (rv == 0) ? HANDLE_FROM_VP(s) : -rv;
resp = malloc (sizeof (sqlc_handle_ct));
resp->result = (rv == 0) ? 0 : -rv;
resp->handle = HANDLE_FROM_VP(s);

return resp;
}

/** FUTURE TBD (???) for sqlcipher:
Expand Down
12 changes: 9 additions & 3 deletions native/sqlc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,20 @@ typedef long long sqlc_long_t;
/* negative number indicates an error: */
typedef sqlc_long_t sqlc_handle_t;

#pragma once
typedef struct {
int result;
sqlc_handle_t handle;
}sqlc_handle_ct;

/* RECOMMENDED (alt 1): Use this call at startup to check Java/native library match
* (returns SQLC_RESULT_OK [0] if OK, other value in case of mismatch) */
int sqlc_api_version_check(int sqlc_api_version);

/* RECOMMENDED (alt 2): Check Java/native library match and open database handle */
sqlc_handle_t sqlc_api_db_open(int sqlc_api_version, const char *filename, int flags);
sqlc_handle_ct* sqlc_api_db_open(int sqlc_api_version, const char *filename, int flags);

sqlc_handle_t sqlc_db_open(const char *filename, int flags);
sqlc_handle_ct* sqlc_db_open(const char *filename, int flags);

// FUTURE TBD (???):
//sqlc_handle_t sqlc_db_open_vfs(const char *filename, int flags, const char *vfs);
Expand All @@ -59,7 +65,7 @@ int sqlc_db_key_native_string(sqlc_handle_t db, char *key_string);
// FUTURE TBD (???) for sqlcipher:
// int sqlc_db_rekey_string_native(sqlc_handle_t db, char *key_string);

sqlc_handle_t sqlc_db_prepare_st(sqlc_handle_t db, const char *sql);
sqlc_handle_ct* sqlc_db_prepare_st(sqlc_handle_t db, const char *sql);

sqlc_long_t sqlc_db_last_insert_rowid(sqlc_handle_t db);
int sqlc_db_total_changes(sqlc_handle_t db);
Expand Down
2 changes: 1 addition & 1 deletion sqlite-amalgamation
Open in desktop
Submodule sqlite-amalgamation updated 4 files
+1 −1 README.md
+39,667 −23,568 sqlite3.c
+1,256 −259 sqlite3.h
+55 −5 sqlite3ext.h