task_core_backup: a backup or restore refused says so, or is taken

Two backups or restores of the same core may not run at once, and a
second one was refused without a word. It was also refused while the
first had finished but not yet retired - its callback still to run -
so Backup pressed again just after one completed did nothing either,
and the finder read the finished task's handle to decide.

A finished backup or restore is passed over now, before its handle is
read: its file work is done, and a second one is taken. One refused
because the same core's is still running puts "A backup or restore of
this core is already running" on screen. Sideload Core flushed the
menu to the top even when its restore was refused, leaving the user
somewhere else with nothing done; it flushes only for a restore that
started, as Restore Backup already did.

core_backup_io_test's findable lane now expects the second backup to
be taken while the first is finished but unretired, and a new lane
checks that a backup of a core whose backup is queued is refused with
a message on screen. Both fail on the old code.

intl/msg_hash_us.h

@@ -34408,6 +34408,10 @@ MSG_HASH(
    MSG_CORE_RESTORATION_DISABLED,
    "Core restoration disabled - core is locked: "
    )
+MSG_HASH(
+   MSG_CORE_BACKUP_IN_PROGRESS,
+   "A backup or restore of this core is already running: "
+   )
 MSG_HASH(
    MSG_CORE_INSTALLATION_DISABLED,
    "Core installation disabled - core is locked: "

menu/cbs/menu_cbs_ok.c

@@ -6340,6 +6340,7 @@ static int action_ok_sideload_core(const char *path,
 {
    const char *menu_path    = NULL;
    bool core_loaded         = false;
+   bool pushed;
    menu_handle_t *menu      = menu_state_get_ptr()->driver_data;
    settings_t *settings     = config_get_ptr();
    const char *dir_libretro = settings->paths.directory_libretro;
@@ -6356,16 +6357,19 @@ static int action_ok_sideload_core(const char *path,
       char backup_path[PATH_MAX_LENGTH];
       fill_pathname_join_special(
             backup_path, menu_path, path, sizeof(backup_path));
-      task_push_core_restore(backup_path, dir_libretro, &core_loaded);
+      pushed = task_push_core_restore(backup_path, dir_libretro, &core_loaded);
    }
    else
-      task_push_core_restore(path, dir_libretro, &core_loaded);
+      pushed = task_push_core_restore(path, dir_libretro, &core_loaded);
 
    /* Flush stack
     * > Since the 'sideload core' option is present
     *   in several locations, can't flush to a predefined
-    *   level - just go to the top */
-   menu_entries_flush_stack(NULL, 0);
+    *   level - just go to the top
+    * > Only for a sideload that started: one refused leaves the
+    *   user where they were, with the reason on screen */
+   if (pushed)
+      menu_entries_flush_stack(NULL, 0);
 
    return 0;
 }

msg_hash.h

@@ -27907,6 +27907,7 @@ enum msg_hash_enums
    MSG_CORE_RESTORATION_FAILED,
    MSG_CORE_INSTALLATION_FAILED,
    MSG_CORE_RESTORATION_DISABLED,
+   MSG_CORE_BACKUP_IN_PROGRESS,
    MSG_CORE_INSTALLATION_DISABLED,
 
    MENU_LABEL(DISK_INDEX),

samples/tasks/core_backup/core_backup_io_test.c

@@ -244,13 +244,18 @@ bool retroarch_ctl(enum rarch_ctl_state state, void *data)
    return false;
 }
 
+/* What was last put on screen */
+static int  g_osd_calls;
+static char g_osd_last[256];
 void runloop_msg_queue_push(const char *msg, size_t len,
       unsigned prio, unsigned duration, bool flush,
       char *title, enum message_queue_icon icon,
       enum message_queue_category category)
 {
-   (void)msg; (void)len; (void)prio; (void)duration; (void)flush;
+   (void)len; (void)prio; (void)duration; (void)flush;
    (void)title; (void)icon; (void)category;
+   g_osd_calls++;
+   strlcpy(g_osd_last, msg ? msg : "", sizeof(g_osd_last));
 }
 
 core_info_t *core_info_find_stub(void) { return NULL; }
@@ -1151,9 +1156,10 @@ static void test_restore_through_symlink(void)
  * finder reading freed memory for the whole stretch between the
  * worker finishing the task and the main thread retiring it.  The
  * lane opens that stretch deliberately - threaded queue, no
- * task_queue_check() - and pushes the duplicate into it; ASan is the
- * assertion for the read, and the push must be refused while the
- * first backup is still findable and accepted once it is retired. */
+ * task_queue_check() - and pushes a second backup into it; ASan is
+ * the assertion for the read.  A finished backup has done its work,
+ * and the second must be taken: it was refused without a word, so
+ * Backup pressed again just after one completed did nothing. */
 static bool finished_task_finder(retro_task_t *task, void *user_data)
 {
    (void)user_data;
@@ -1221,10 +1227,10 @@ static void test_finished_task_stays_findable(void)
       retro_sleep(1);
    CHECK(i < 10000, "backup task did not finish");
 
-   CHECK(!task_push_core_backup(g_core_path, "Test Core", 0,
+   CHECK(task_push_core_backup(g_core_path, "Test Core", 0,
             CORE_BACKUP_MODE_MANUAL, 0, g_assets_dir, true,
-            NULL, NULL),
-         "duplicate backup accepted while the first is still findable");
+            NULL, NULL) != NULL,
+         "a backup refused because the finished one was not yet retired");
 
    CHECK(g_done_calls == 0, "completion callback ran before retirement");
 
@@ -1245,6 +1251,41 @@ static void test_finished_task_stays_findable(void)
    task_queue_unset_threaded();
 }
 
+/* A backup of a core whose backup has not run yet: refused, as two at
+ * once are not allowed, and the user is told why instead of the entry
+ * doing nothing. */
+static void test_running_backup_refused_and_told(void)
+{
+   uint8_t *payload;
+   size_t core_size = 64 * 1024;
+
+   printf("  a backup of a core being backed up is refused, and said\n");
+   if (!(payload = make_payload(core_size)))
+      return;
+   if (!write_file_bytes(g_core_path, payload, core_size))
+   {
+      free(payload);
+      return;
+   }
+   free(payload);
+
+   g_clock_now  = 1000000;
+   g_clock_step = 0;
+   task_queue_init(false, NULL);
+   CHECK(task_push_core_backup(g_core_path, "Test Core", 0,
+            CORE_BACKUP_MODE_MANUAL, 0, g_assets_dir, true,
+            NULL, NULL) != NULL, "the first backup was not pushed");
+   g_osd_calls   = 0;
+   g_osd_last[0] = '\0';
+   CHECK(!task_push_core_backup(g_core_path, "Test Core", 0,
+            CORE_BACKUP_MODE_MANUAL, 0, g_assets_dir, true,
+            NULL, NULL), "a second backup of the same core was taken");
+   CHECK(g_osd_calls == 1 && *g_osd_last,
+         "the refused backup said nothing on screen");
+   task_queue_wait(NULL, NULL);
+   task_queue_deinit();
+}
+
 int main(int argc, char **argv)
 {
    (void)argc; (void)argv;
@@ -1296,6 +1337,7 @@ int main(int argc, char **argv)
 
    printf("\n[task lifetime]\n");
    test_finished_task_stays_findable();
+   test_running_backup_refused_and_told();
 
    printf("\n%s (%d check%s, %d failure%s)\n",
          failures ? "FAILED" : "PASSED",

tasks/task_core_backup.c

@@ -297,6 +297,13 @@ static bool task_core_backup_finder(retro_task_t *task, void *user_data)
        && (task->handler != task_core_restore_handler))
       return false;
 
+   /* A finished backup or restore has done its file work, though it
+    * stays in the queue until its callback has run: a second one of
+    * the same core pushed meanwhile was refused without a word. Passed
+    * over before its handle is read, which may already be gone. */
+   if (task_get_flags(task) & RETRO_TASK_FLG_FINISHED)
+      return false;
+
    backup_handle = (core_backup_handle_t*)task->state;
    if (!backup_handle)
       return false;
@@ -314,6 +321,29 @@ static bool task_core_backup_finder(retro_task_t *task, void *user_data)
    return string_is_equal(core_filename_a, core_filename_b);
 }
 
+/* Concurrent backup/restore tasks for the same core are not allowed:
+ * true when one is running, and the user is told rather than the
+ * Backup or Restore entry doing nothing. */
+static bool task_core_backup_busy(const char *core_path)
+{
+   task_finder_data_t find_data;
+   find_data.func     = task_core_backup_finder;
+   find_data.userdata = (void*)core_path;
+
+   if (task_queue_find(&find_data))
+   {
+      char msg[128];
+      size_t _len = strlcpy(msg,
+            msg_hash_to_str(MSG_CORE_BACKUP_IN_PROGRESS), sizeof(msg));
+      strlcpy(msg + _len, path_basename(core_path), sizeof(msg) - _len);
+      RARCH_LOG("[Core Backup] %s\n", msg);
+      runloop_msg_queue_push(msg, strlen(msg), 1, 100, true, NULL,
+            MESSAGE_QUEUE_ICON_DEFAULT, MESSAGE_QUEUE_CATEGORY_INFO);
+      return true;
+   }
+   return false;
+}
+
 /***************/
 /* Core Backup */
 /***************/
@@ -843,7 +873,6 @@ static void *task_push_core_backup_internal(
       retro_task_callback_t cb, void *user_data)
 {
    size_t _len;
-   task_finder_data_t find_data;
    const char *core_name               = NULL;
    retro_task_t *task                  = NULL;
    core_backup_handle_t *backup_handle = NULL;
@@ -856,10 +885,7 @@ static void *task_push_core_backup_internal(
 
    /* Concurrent backup/restore tasks for the same core
     * are not allowed */
-   find_data.func     = task_core_backup_finder;
-   find_data.userdata = (void*)core_path;
-
-   if (task_queue_find(&find_data))
+   if (task_core_backup_busy(core_path))
       goto error;
 
    /* Get core name */
@@ -1481,7 +1507,6 @@ bool task_push_core_restore(const char *backup_path, const char *dir_libretro,
       bool *core_loaded)
 {
    size_t _len;
-   task_finder_data_t find_data;
    enum core_backup_type backup_type;
    core_info_t *core_info              = NULL;
    const char *core_name               = NULL;
@@ -1567,10 +1592,7 @@ bool task_push_core_restore(const char *backup_path, const char *dir_libretro,
 
    /* Concurrent backup/restore tasks for the same core
     * are not allowed */
-   find_data.func     = task_core_backup_finder;
-   find_data.userdata = (void*)core_path;
-
-   if (task_queue_find(&find_data))
+   if (task_core_backup_busy(core_path))
       goto error;
 
    /* Configure handle */