fix broken external ioengine option

get_engine_name() function added to support external ioengine option
via "external:" prefix in below commits in 2007 has been broken.
 7b395ca5('Prefix external io engine loading with 'external'')
 8a7bd877('Document loading external io engines')

It seems to have been broken since below commit in 2011 which made
__handle_option() strip ':' and after while parsing the option.
 c44b1ff5('Add sub-option support (sort-of) and convert libaio_userspace_reap')

Above change made fio dlopen "external" instead of the path located
after "external:", since the path had already been stripped by
__handle_option() by the time get_engine_name() was called. This commit
fixes it by adding ->ioengine_so_path pointer to keep the path while
parsing using sub-option callback.

Note that removing get_engine_name() doesn't affect non "external:"
ioengine options, as the option parser also strips while parsing.

-- before this commit
 # ./fio --name=xxx --ioengine=external:./engines/skeleton_external.so
 fio: engine external not loadable
 fio: failed to load engine external
 fio: file:ioengines.c:91, func=dlopen, error=external: cannot open shared object file: No such file or directory

-- with this commit
 # ./fio --name=xxx --ioengine=external:./engines/skeleton_external.so
 xxx: (g=0): rw=read, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=engine_name, iodepth=1
 fio-3.0-32-g68f6f
 Starting 1 process
 xxx: you need to specify size=
 fio: pid=0, err=22/file:filesetup.c:973, func=total_file_size, error=Invalid argument

 Run status group 0 (all jobs):

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/init.c b/init.c
index 625c937141d1f029d1bbae70da2de1b8df12aded..2da64ba89e1c0005f15b80a792efddaf0636e510 100644 (file)
--- a/init.c
+++ b/init.c
@@ -912,20 +912,6 @@ static int fixup_options(struct thread_data *td)
        return ret;
 }
 
        return ret;
 }
 

-/* External engines are specified by "external:name.o") */
-static const char *get_engine_name(const char *str)
-{
-       char *p = strstr(str, ":");
-
-       if (!p)
-               return str;
-
-       p++;
-       strip_blank_front(&p);
-       strip_blank_end(p);
-       return p;
-}
-

 static void init_rand_file_service(struct thread_data *td)
 {
        unsigned long nranges = td->o.nr_files << FIO_FSERVICE_SHIFT;
 static void init_rand_file_service(struct thread_data *td)
 {
        unsigned long nranges = td->o.nr_files << FIO_FSERVICE_SHIFT;


@@ -1057,7 +1043,10 @@ int ioengine_load(struct thread_data *td)
                free_ioengine(td);
        }
 
                free_ioengine(td);
        }
 

-       engine = get_engine_name(td->o.ioengine);
+       /*
+        * Use ->ioengine_so_path if an external ioengine is specified.
+        */
+       engine = td->o.ioengine_so_path ?: td->o.ioengine;

        td->io_ops = load_ioengine(td, engine);
        if (!td->io_ops) {
                log_err("fio: failed to load engine %s\n", engine);
        td->io_ops = load_ioengine(td, engine);
        if (!td->io_ops) {
                log_err("fio: failed to load engine %s\n", engine);

diff --git a/options.c b/options.c
index 443791abc6b1dc46400ec057abc0c03e83ea8023..54fa4eef7ac5965792ff0fae3aeeaad85470a7c5 100644 (file)
--- a/options.c
+++ b/options.c
@@ -1462,6 +1462,39 @@ static int str_write_hist_log_cb(void *data, const char *str)
        return 0;
 }
 
        return 0;
 }
 

+/*
+ * str is supposed to be a substring of the strdup'd original string,
+ * and is valid only if it's a regular file path.
+ * This function keeps the pointer to the path as needed later.
+ *
+ * "external:/path/to/so\0" <- original pointer updated with strdup'd
+ * "external\0"             <- above pointer after parsed, i.e. ->ioengine
+ *          "/path/to/so\0" <- str argument, i.e. ->ioengine_so_path
+ */
+static int str_ioengine_external_cb(void *data, const char *str)
+{
+       struct thread_data *td = cb_data_to_td(data);
+       struct stat sb;
+       char *p;
+
+       if (!str) {
+               log_err("fio: null external ioengine path\n");
+               return 1;
+       }
+
+       p = (char *)str; /* str is mutable */
+       strip_blank_front(&p);
+       strip_blank_end(p);
+
+       if (stat(p, &sb) || !S_ISREG(sb.st_mode)) {
+               log_err("fio: invalid external ioengine path \"%s\"\n", p);
+               return 1;
+       }
+
+       td->o.ioengine_so_path = p;
+       return 0;
+}
+

 static int rw_verify(struct fio_option *o, void *data)
 {
        struct thread_data *td = cb_data_to_td(data);
 static int rw_verify(struct fio_option *o, void *data)
 {
        struct thread_data *td = cb_data_to_td(data);


@@ -1812,6 +1845,7 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
 #endif
                          { .ival = "external",
                            .help = "Load external engine (append name)",
 #endif
                          { .ival = "external",
                            .help = "Load external engine (append name)",

+                           .cb = str_ioengine_external_cb,

                          },
                },
        },
                          },
                },
        },

diff --git a/thread_options.h b/thread_options.h
index 26a3e0e67c4968098319bca119a9c26d7ec5efa8..fd6576e252c75ae3e294b6cf6f5c449486ca2d90 100644 (file)
--- a/thread_options.h
+++ b/thread_options.h
@@ -53,6 +53,7 @@ struct thread_options {
        char *filename_format;
        char *opendir;
        char *ioengine;
        char *filename_format;
        char *opendir;
        char *ioengine;

+       char *ioengine_so_path;

        char *mmapfile;
        enum td_ddir td_ddir;
        unsigned int rw_seq;
        char *mmapfile;
        enum td_ddir td_ddir;
        unsigned int rw_seq;