Re: [Spice-devel] [PATCH v2 spice-streaming-agent] Allow to set plugins directory via command line
Hi, Pushed :/ , sorry we should have wait a bit more for more comments.. On 02/22/2018 04:14 PM, Christophe de Dinechin wrote: On 22 Feb 2018, at 14:15, Snir Sheriberwrote: Could be useful mainly for testing purposes, it allows to load plugins that aren't installed in the default plugins folder. To set different plugins directory use --plugins-dir= Signed-off-by: Snir Sheriber --- src/spice-streaming-agent.cpp | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp index 267b76e..9c04576 100644 --- a/src/spice-streaming-agent.cpp +++ b/src/spice-streaming-agent.cpp @@ -283,6 +283,7 @@ static void usage(const char *progname) printf("\t-p portname -- virtio-serial port to use\n"); printf("\t-l file -- log frames to file\n"); printf("\t--log-binary -- log binary frames (following -l)\n"); +printf("\t--plugins-dir=path -- change plugins directory\n"); printf("\t-d -- enable debug logs\n"); printf("\t-c variable=value -- change settings\n"); printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n"); @@ -445,10 +446,15 @@ done: int main(int argc, char* argv[]) { const char *streamport = "/dev/virtio-ports/com.redhat.stream.0"; -char opt; +int opt; const char *log_filename = NULL; int logmask = LOG_UPTO(LOG_WARNING); +const char *pluginsdir = PLUGINSDIR; +enum { +OPT_PLUGINS_DIR = UCHAR_MAX+1 +}; I thought Frediano already had a patch that did that (for the log-binary option). Watch out for conflicts. In order to help the resolution of such conflict, what about avoiding to have = on option names, i.e. enum { OPT_FIRST = UCHAR_MAX, OPT_PLUGINS_DIR } (That way, if we merge with something that has OPT_LOG_BINARY, they don’t both have “OPT_X = same value”) He had but was not pushed, not sure why - part of a series? Could be done this way but I don't see it as a big issue, OPT_PLUGINS_DIR will just follow. struct option long_options[] = { +{ "plugins-dir", required_argument, NULL, OPT_PLUGINS_DIR}, { "log-binary", no_argument, _binary, 1}, { "help", no_argument, NULL, 'h'}, { 0, 0, 0, 0} @@ -464,6 +470,10 @@ int main(int argc, char* argv[]) case 0: /* Handle long options if needed */ break; +case OPT_PLUGINS_DIR: much better, thanks. +if (optarg) +pluginsdir = optarg; else … 1) option error, we want a plugin directory argument 2) in that error, show the default plugin-dir, might be useful to extract a compile-time constant from a binary easily. If was removed- should never happen, though we can check for the validity of the path.. (too careful? idk) +break; case 'p': streamport = optarg; break; @@ -493,7 +503,7 @@ int main(int argc, char* argv[]) // register built-in plugins MjpegPlugin::Register(); -agent.LoadPlugins(PLUGINSDIR); +agent.LoadPlugins(pluginsdir); register_interrupts(); -- 2.14.3 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 spice-streaming-agent] Allow to set plugins directory via command line
> On 22 Feb 2018, at 14:15, Snir Sheriberwrote: > > Could be useful mainly for testing purposes, it allows to load > plugins that aren't installed in the default plugins folder. > To set different plugins directory use --plugins-dir= > > Signed-off-by: Snir Sheriber > --- > src/spice-streaming-agent.cpp | 14 -- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp > index 267b76e..9c04576 100644 > --- a/src/spice-streaming-agent.cpp > +++ b/src/spice-streaming-agent.cpp > @@ -283,6 +283,7 @@ static void usage(const char *progname) > printf("\t-p portname -- virtio-serial port to use\n"); > printf("\t-l file -- log frames to file\n"); > printf("\t--log-binary -- log binary frames (following -l)\n"); > +printf("\t--plugins-dir=path -- change plugins directory\n"); > printf("\t-d -- enable debug logs\n"); > printf("\t-c variable=value -- change settings\n"); > printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n"); > @@ -445,10 +446,15 @@ done: > int main(int argc, char* argv[]) > { > const char *streamport = "/dev/virtio-ports/com.redhat.stream.0"; > -char opt; > +int opt; > const char *log_filename = NULL; > int logmask = LOG_UPTO(LOG_WARNING); > +const char *pluginsdir = PLUGINSDIR; > +enum { > +OPT_PLUGINS_DIR = UCHAR_MAX+1 > +}; I thought Frediano already had a patch that did that (for the log-binary option). Watch out for conflicts. In order to help the resolution of such conflict, what about avoiding to have = on option names, i.e. enum { OPT_FIRST = UCHAR_MAX, OPT_PLUGINS_DIR } (That way, if we merge with something that has OPT_LOG_BINARY, they don’t both have “OPT_X = same value”) > struct option long_options[] = { > +{ "plugins-dir", required_argument, NULL, OPT_PLUGINS_DIR}, > { "log-binary", no_argument, _binary, 1}, > { "help", no_argument, NULL, 'h'}, > { 0, 0, 0, 0} > @@ -464,6 +470,10 @@ int main(int argc, char* argv[]) > case 0: > /* Handle long options if needed */ > break; > +case OPT_PLUGINS_DIR: much better, thanks. > +if (optarg) > +pluginsdir = optarg; else … 1) option error, we want a plugin directory argument 2) in that error, show the default plugin-dir, might be useful to extract a compile-time constant from a binary easily. > +break; > case 'p': > streamport = optarg; > break; > @@ -493,7 +503,7 @@ int main(int argc, char* argv[]) > // register built-in plugins > MjpegPlugin::Register(); > > -agent.LoadPlugins(PLUGINSDIR); > +agent.LoadPlugins(pluginsdir); > > register_interrupts(); > > -- > 2.14.3 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 spice-streaming-agent] Allow to set plugins directory via command line
> > Could be useful mainly for testing purposes, it allows to load > plugins that aren't installed in the default plugins folder. > To set different plugins directory use --plugins-dir= > > Signed-off-by: Snir Sheriber> --- > src/spice-streaming-agent.cpp | 14 -- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp > index 267b76e..9c04576 100644 > --- a/src/spice-streaming-agent.cpp > +++ b/src/spice-streaming-agent.cpp > @@ -283,6 +283,7 @@ static void usage(const char *progname) > printf("\t-p portname -- virtio-serial port to use\n"); > printf("\t-l file -- log frames to file\n"); > printf("\t--log-binary -- log binary frames (following -l)\n"); > +printf("\t--plugins-dir=path -- change plugins directory\n"); > printf("\t-d -- enable debug logs\n"); > printf("\t-c variable=value -- change settings\n"); > printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n"); > @@ -445,10 +446,15 @@ done: > int main(int argc, char* argv[]) > { > const char *streamport = "/dev/virtio-ports/com.redhat.stream.0"; > -char opt; > +int opt; > const char *log_filename = NULL; > int logmask = LOG_UPTO(LOG_WARNING); > +const char *pluginsdir = PLUGINSDIR; > +enum { > +OPT_PLUGINS_DIR = UCHAR_MAX+1 > +}; > struct option long_options[] = { > +{ "plugins-dir", required_argument, NULL, OPT_PLUGINS_DIR}, > { "log-binary", no_argument, _binary, 1}, > { "help", no_argument, NULL, 'h'}, > { 0, 0, 0, 0} > @@ -464,6 +470,10 @@ int main(int argc, char* argv[]) > case 0: > /* Handle long options if needed */ > break; > +case OPT_PLUGINS_DIR: > +if (optarg) > +pluginsdir = optarg; style (brackets). However as you specified required_argument I would just remove the if statement. > +break; > case 'p': > streamport = optarg; > break; > @@ -493,7 +503,7 @@ int main(int argc, char* argv[]) > // register built-in plugins > MjpegPlugin::Register(); > > -agent.LoadPlugins(PLUGINSDIR); > +agent.LoadPlugins(pluginsdir); > > register_interrupts(); > Otherwise, Acked-by: Frediano Ziglio Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel