From 8518d8f456f418376b17cf69386b604d369f4c63 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Wed, 25 Sep 2019 02:52:12 +0200 Subject: [PATCH] rustify imex and friends --- deltachat-ffi/src/lib.rs | 10 +- python/src/deltachat/account.py | 32 +++--- python/tests/test_account.py | 9 +- src/constants.rs | 8 +- src/dc_imex.rs | 179 +++++++++++++------------------- src/dc_tools.rs | 7 +- src/job.rs | 7 +- 7 files changed, 122 insertions(+), 130 deletions(-) diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index aec335d13..bd41e32e7 100644 --- a/deltachat-ffi/src/lib.rs +++ b/deltachat-ffi/src/lib.rs @@ -1527,7 +1527,7 @@ pub unsafe extern "C" fn dc_imex( } let ffi_context = &*context; ffi_context - .with_inner(|ctx| dc_imex::dc_imex(ctx, what, as_opt_str(param1), param2)) + .with_inner(|ctx| dc_imex::dc_imex(ctx, what, as_opt_str(param1), as_opt_str(param2))) .ok(); } @@ -1542,7 +1542,13 @@ pub unsafe extern "C" fn dc_imex_has_backup( } let ffi_context = &*context; ffi_context - .with_inner(|ctx| dc_imex::dc_imex_has_backup(ctx, as_str(dir))) + .with_inner(|ctx| match dc_imex::dc_imex_has_backup(ctx, as_str(dir)) { + Ok(res) => res.strdup(), + Err(err) => { + error!(ctx, "dc_imex_has_backup: {}", err); + ptr::null_mut() + } + }) .unwrap_or_else(|_| ptr::null_mut()) } diff --git a/python/src/deltachat/account.py b/python/src/deltachat/account.py index b799d23b5..5445c4906 100644 --- a/python/src/deltachat/account.py +++ b/python/src/deltachat/account.py @@ -48,7 +48,7 @@ class Account(object): if not lib.dc_open(self._dc_context, db_path, ffi.NULL): raise ValueError("Could not dc_open: {}".format(db_path)) self._configkeys = self.get_config("sys.config_keys").split() - self._imex_progress_finished = Queue() + self._imex_events = Queue() def __del__(self): self.shutdown() @@ -303,24 +303,28 @@ class Account(object): raise RuntimeError("found more than one new file") return l[0] - def _imex_progress_finished_clear(self): + def _imex_events_clear(self): try: while True: - self._imex_progress_finished.get_nowait() + self._imex_events.get_nowait() except Empty: pass def _export(self, path, imex_cmd): snap_files = os.listdir(path) - self._imex_progress_finished_clear() + self._imex_events_clear() lib.dc_imex(self._dc_context, imex_cmd, as_dc_charpointer(path), ffi.NULL) if not self._threads.is_started(): lib.dc_perform_imap_jobs(self._dc_context) - if not self._imex_progress_finished.get(): - raise ValueError("export failed") - return [ os.path.join(path, x) - for x in os.listdir(backupdir) - if x not in snap_files] + files_written = [] + while True: + ev = self._imex_events.get() + if isinstance(ev, str): + files_written.append(ev) + elif isinstance(ev, bool): + if not ev: + raise ValueError("export failed, exp-files: {}".format(files_written)) + return files_written def import_self_keys(self, path): """ Import private keys found in the `path` directory. @@ -338,11 +342,11 @@ class Account(object): self._import(path, imex_cmd=12) def _import(self, path, imex_cmd): - self._imex_progress_finished_clear() + self._imex_events_clear() lib.dc_imex(self._dc_context, imex_cmd, as_dc_charpointer(path), ffi.NULL) if not self._threads.is_started(): lib.dc_perform_imap_jobs(self._dc_context) - if not self._imex_progress_finished.get(): + if not self._imex_events.get(): raise ValueError("import from path '{}' failed".format(path)) def initiate_key_transfer(self): @@ -453,10 +457,12 @@ class Account(object): def on_dc_event_imex_progress(self, data1, data2): if data1 == 1000: - self._imex_progress_finished.put(True) + self._imex_events.put(True) elif data1 == 0: - self._imex_progress_finished.put(False) + self._imex_events.put(False) + def on_dc_event_imex_file_written(self, data1, data2): + self._imex_events.put(data1) class IOThreads: def __init__(self, dc_context, log_event=lambda *args: None): diff --git a/python/tests/test_account.py b/python/tests/test_account.py index 45c0a585d..bf9dfb61e 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -343,9 +343,12 @@ class TestOnlineAccount: def test_export_import_self_keys(self, acfactory, tmpdir): ac1, ac2 = acfactory.get_two_online_accounts() dir = tmpdir.mkdir("exportdir") - tmpfile = ac1.export_self_keys(dir.strpath) - ac1.consume_events() - assert ac1.import_self_keys(tmpfile) + l = ac1.export_self_keys(dir.strpath) + assert len(l) == 2 + for x in l: + assert x.startswith(dir.strpath) + ac1._evlogger.consume_events() + ac1.import_self_keys(dir.strpath) def test_one_account_send(self, acfactory): ac1 = acfactory.get_online_configuring_account() diff --git a/src/constants.rs b/src/constants.rs index a67495607..f9771a8c0 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -65,13 +65,13 @@ pub(crate) const DC_FP_NO_AUTOCRYPT_HEADER: i32 = 2; pub(crate) const DC_FP_ADD_AUTOCRYPT_HEADER: i32 = 1; /// param1 is a directory where the keys are written to -const DC_IMEX_EXPORT_SELF_KEYS: usize = 1; +pub const DC_IMEX_EXPORT_SELF_KEYS: i32 = 1; /// param1 is a directory where the keys are searched in and read from -const DC_IMEX_IMPORT_SELF_KEYS: usize = 2; +pub const DC_IMEX_IMPORT_SELF_KEYS: i32 = 2; /// param1 is a directory where the backup is written to -const DC_IMEX_EXPORT_BACKUP: usize = 11; +pub const DC_IMEX_EXPORT_BACKUP: i32 = 11; /// param1 is the file with the backup to import -const DC_IMEX_IMPORT_BACKUP: usize = 12; +pub const DC_IMEX_IMPORT_BACKUP: i32 = 12; /// virtual chat showing all messages belonging to chats flagged with chats.blocked=2 pub(crate) const DC_CHAT_ID_DEADDROP: u32 = 1; diff --git a/src/dc_imex.rs b/src/dc_imex.rs index 96c6f2c3e..e6e0a28f3 100644 --- a/src/dc_imex.rs +++ b/src/dc_imex.rs @@ -34,15 +34,15 @@ pub fn dc_imex( context: &Context, what: libc::c_int, param1: Option>, - param2: *const libc::c_char, + param2: Option>, ) { let mut param = Params::new(); param.set_int(Param::Cmd, what as i32); if let Some(param1) = param1 { param.set(Param::Arg, param1.as_ref().to_string_lossy()); } - if !param2.is_null() { - param.set(Param::Arg2, as_str(param2)); + if let Some(param2) = param2 { + param.set(Param::Arg, param2.as_ref().to_string_lossy()); } job_kill_action(context, Action::ImexImap); @@ -50,19 +50,11 @@ pub fn dc_imex( } /// Returns the filename of the backup if found, nullptr otherwise. -pub unsafe fn dc_imex_has_backup( - context: &Context, - dir_name: impl AsRef, -) -> *mut libc::c_char { +pub fn dc_imex_has_backup(context: &Context, dir_name: impl AsRef) -> Result { let dir_name = dir_name.as_ref(); let dir_iter = std::fs::read_dir(dir_name); if dir_iter.is_err() { - info!( - context, - "Backup check: Cannot open directory \"{}\".\x00", - dir_name.display(), - ); - return ptr::null_mut(); + bail!("Backup check: Cannot open directory \"{:?}\"", dir_name); } let mut newest_backup_time = 0; let mut newest_backup_path: Option = None; @@ -89,14 +81,8 @@ pub unsafe fn dc_imex_has_backup( } } match newest_backup_path { - Some(path) => match path.to_c_string() { - Ok(cstr) => dc_strdup(cstr.as_ptr()), - Err(err) => { - error!(context, "Invalid backup filename: {}", err); - std::ptr::null_mut() - } - }, - None => std::ptr::null_mut(), + Some(path) => Ok(path.to_string_lossy().into_owned()), + None => bail!("no backup found"), } } @@ -450,71 +436,54 @@ pub fn dc_normalize_setup_code(s: &str) -> String { } #[allow(non_snake_case)] -pub unsafe fn dc_job_do_DC_JOB_IMEX_IMAP(context: &Context, job: &Job) { - let mut ok_to_continue = true; - let mut success: libc::c_int = 0; - - if dc_alloc_ongoing(context) { - let what = job.param.get_int(Param::Cmd).unwrap_or_default(); - let param1_s = job.param.get(Param::Arg).unwrap_or_default(); - let param1 = CString::yolo(param1_s); - let _param2 = CString::yolo(job.param.get(Param::Arg2).unwrap_or_default()); - - if strlen(param1.as_ptr()) == 0 { - error!(context, "No Import/export dir/file given.",); - } else { - info!(context, "Import/export process started.",); - context.call_cb(Event::ImexProgress(10)); - - if !context.sql.is_open() { - error!(context, "Import/export: Database not opened.",); - } else { - if what == 1 || what == 11 { - /* before we export anything, make sure the private key exists */ - if e2ee::ensure_secret_key_exists(context).is_err() { - error!( - context, - "Import/export: Cannot create private key or private key not available.", - ); - ok_to_continue = false; - } else { - dc_create_folder(context, ¶m1_s); - } - } - if ok_to_continue { - match what { - 1 => { - if export_self_keys(context, param1.as_ptr()) { - info!(context, "Import/export completed.",); - success = 1 - } - } - 2 => { - if 0 != import_self_keys(context, param1.as_ptr()) { - info!(context, "Import/export completed.",); - success = 1 - } - } - 11 => { - if export_backup(context, param1.as_ptr()) { - info!(context, "Import/export completed.",); - success = 1 - } - } - 12 => { - if import_backup(context, param1.as_ptr()) { - info!(context, "Import/export completed.",); - success = 1 - } - } - _ => {} - } - } - } - } - dc_free_ongoing(context); +pub fn dc_job_do_DC_JOB_IMEX_IMAP(context: &Context, job: &Job) -> Result<()> { + if !dc_alloc_ongoing(context) { + bail!("could not allocate ongoing") + } + let what = job.param.get_int(Param::Cmd).unwrap_or_default(); + let param1_s = job.param.get(Param::Arg).unwrap_or_default(); + let param1 = CString::yolo(param1_s); + let _param2 = CString::yolo(job.param.get(Param::Arg2).unwrap_or_default()); + + if param1_s.empty() == 0 { + bail!("No Import/export dir/file given."); + } + info!(context, "Import/export process started."); + context.call_cb(Event::ImexProgress(10)); + + if !context.sql.is_open() { + bail!("Database not opened."); + } + if what == DC_IMEX_EXPORT_BACKUP || what == DC_IMEX_EXPORT_SELF_KEYS { + /* before we export anything, make sure the private key exists */ + if e2ee::ensure_secret_key_exists(context).is_err() { + dc_free_ongoing(context); + bail!("Cannot create private key or private key not available."); + } else { + dc_create_folder(context, ¶m1_s); + } + } + let success = match what { + DC_IMEX_EXPORT_SELF_KEYS => unsafe { export_self_keys(context, param1.as_ptr()) }, + DC_IMEX_IMPORT_SELF_KEYS => unsafe { import_self_keys(context, param1.as_ptr()) }, + DC_IMEX_EXPORT_BACKUP => unsafe { export_backup(context, param1.as_ptr()) }, + DC_IMEX_IMPORT_BACKUP => unsafe { import_backup(context, param1.as_ptr()) }, + _ => { + bail!("unknown IMEX type: {}", what); + } + }; + dc_free_ongoing(context); + match success { + true => { + info!(context, "IMEX successfully completed"); + context.call_cb(Event::ImexProgress(1000)); + Ok(()) + } + false => { + context.call_cb(Event::ImexProgress(0)); + bail!("IMEX FAILED to complete"); + } } - context.call_cb(Event::ImexProgress(if 0 != success { 1000 } else { 0 })); } /******************************************************************************* @@ -805,7 +774,7 @@ unsafe fn export_backup(context: &Context, dir: *const libc::c_char) -> bool { /******************************************************************************* * Classic key import ******************************************************************************/ -unsafe fn import_self_keys(context: &Context, dir_name: *const libc::c_char) -> libc::c_int { +unsafe fn import_self_keys(context: &Context, dir_name: *const libc::c_char) -> bool { /* hint: even if we switch to import Autocrypt Setup Files, we should leave the possibility to import plain ASC keys, at least keys without a password, if we do not want to implement a password entry function. Importing ASC keys is useful to use keys in Delta Chat used by any other non-Autocrypt-PGP implementation. @@ -910,7 +879,7 @@ unsafe fn import_self_keys(context: &Context, dir_name: *const libc::c_char) -> free(buf as *mut libc::c_void); free(buf2 as *mut libc::c_void); - imported_cnt + imported_cnt != 0 } unsafe fn export_self_keys(context: &Context, dir: *const libc::c_char) -> bool { @@ -936,14 +905,14 @@ unsafe fn export_self_keys(context: &Context, dir: *const libc::c_char) -> bool let (id, public_key, private_key, is_default) = key_pair?; let id = Some(id).filter(|_| is_default != 0); if let Some(key) = public_key { - if export_key_to_asc_file(context, dir, id, &key) { + if !export_key_to_asc_file(context, dir, id, &key) { export_errors += 1; } } else { export_errors += 1; } if let Some(key) = private_key { - if export_key_to_asc_file(context, dir, id, &key) { + if !export_key_to_asc_file(context, dir, id, &key) { export_errors += 1; } } else { @@ -962,7 +931,7 @@ unsafe fn export_self_keys(context: &Context, dir: *const libc::c_char) -> bool /******************************************************************************* * Classic key export ******************************************************************************/ -unsafe fn export_key_to_asc_file( +fn export_key_to_asc_file( context: &Context, dir: *const libc::c_char, id: Option, @@ -1050,23 +1019,21 @@ mod tests { #[test] fn test_export_key_to_asc_file() { - unsafe { - let context = dummy_context(); - let base64 = include_str!("../test-data/key/public.asc"); - let key = Key::from_base64(base64, KeyType::Public).unwrap(); - let blobdir = CString::yolo("$BLOBDIR"); - assert!(export_key_to_asc_file( - &context.ctx, - blobdir.as_ptr(), - None, - &key - )); - let blobdir = context.ctx.get_blobdir().to_str().unwrap(); - let filename = format!("{}/public-key-default.asc", blobdir); - let bytes = std::fs::read(&filename).unwrap(); + let context = dummy_context(); + let base64 = include_str!("../test-data/key/public.asc"); + let key = Key::from_base64(base64, KeyType::Public).unwrap(); + let blobdir = CString::yolo("$BLOBDIR"); + assert!(export_key_to_asc_file( + &context.ctx, + blobdir.as_ptr(), + None, + &key + )); + let blobdir = context.ctx.get_blobdir().to_str().unwrap(); + let filename = format!("{}/public-key-default.asc", blobdir); + let bytes = std::fs::read(&filename).unwrap(); - assert_eq!(bytes, key.to_asc(None).into_bytes()); - } + assert_eq!(bytes, key.to_asc(None).into_bytes()); } #[test] diff --git a/src/dc_tools.rs b/src/dc_tools.rs index a7fdda9fb..a1c1840f5 100644 --- a/src/dc_tools.rs +++ b/src/dc_tools.rs @@ -530,10 +530,13 @@ pub(crate) fn dc_get_filebytes(context: &Context, path: impl AsRef) -> bool { let path_abs = dc_get_abs_path(context, &path); + if !path_abs.exists() { + return false; + } if !path_abs.is_file() { warn!( context, - "Will not delete directory \"{}\".", + "refusing to deleting non-file \"{}\".", path.as_ref().display() ); return false; @@ -1499,6 +1502,7 @@ mod tests { let t = dummy_context(); let context = &t.ctx; + assert!(!dc_delete_file(context, "$BLOBDIR/lkqwjelqkwlje")); if dc_file_exist(context, "$BLOBDIR/foobar") || dc_file_exist(context, "$BLOBDIR/dada") || dc_file_exist(context, "$BLOBDIR/foobar.dadada") @@ -1521,6 +1525,7 @@ mod tests { .to_string(); assert!(dc_is_blobdir_path(context, &abs_path)); + assert!(dc_is_blobdir_path(context, "$BLOBDIR/fofo",)); assert!(!dc_is_blobdir_path(context, "/BLOBDIR/fofo",)); assert!(dc_file_exist(context, &abs_path)); diff --git a/src/job.rs b/src/job.rs index 9ea797f9a..57e5c1fd0 100644 --- a/src/job.rs +++ b/src/job.rs @@ -824,7 +824,12 @@ fn job_perform(context: &Context, thread: Thread, probe_network: bool) { Action::MoveMsg => job.do_DC_JOB_MOVE_MSG(context), Action::SendMdn => job.do_DC_JOB_SEND(context), Action::ConfigureImap => unsafe { dc_job_do_DC_JOB_CONFIGURE_IMAP(context) }, - Action::ImexImap => unsafe { dc_job_do_DC_JOB_IMEX_IMAP(context, &job) }, + Action::ImexImap => match dc_job_do_DC_JOB_IMEX_IMAP(context, &job) { + Ok(()) => {} + Err(err) => { + error!(context, "{}", err); + } + }, Action::MaybeSendLocations => { location::job_do_DC_JOB_MAYBE_SEND_LOCATIONS(context, &job) }