migration/exec.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
* Use cmd instead of /bin/sh on Windows.
* Try to auto-detect cmd.exe's path, but default to a hard-coded path.
Note that this will require that gspawn-win[32|64]-helper.exe and
gspawn-win[32|64]-helper-console.exe are included in the Windows binary
distributions (cc: Stefan Weil).
Signed-off-by: "John Berberian, Jr" <jeb.study@gmail.com>
---
Whoops, forgot a header. Here's a revised patch.
migration/exec.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/migration/exec.c b/migration/exec.c
index 375d2e1b54..38604d73a6 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -23,12 +23,31 @@
#include "migration.h"
#include "io/channel-command.h"
#include "trace.h"
+#include "qemu/cutils.h"
+#ifdef WIN32
+const char *exec_get_cmd_path(void);
+const char *exec_get_cmd_path(void)
+{
+ g_autofree char *detected_path = g_new(char, MAX_PATH);
+ if (GetSystemDirectoryA(detected_path, MAX_PATH) == 0) {
+ warn_report("Could not detect cmd.exe path, using default.");
+ return "C:\\Windows\\System32\\cmd.exe";
+ }
+ pstrcat(detected_path, MAX_PATH, "\\cmd.exe");
+ return g_steal_pointer(&detected_path);
+}
+#endif
void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
{
QIOChannel *ioc;
+
+#ifdef WIN32
+ const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
+#else
const char *argv[] = { "/bin/sh", "-c", command, NULL };
+#endif
trace_migration_exec_outgoing(command);
ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
@@ -55,7 +74,12 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
void exec_start_incoming_migration(const char *command, Error **errp)
{
QIOChannel *ioc;
+
+#ifdef WIN32
+ const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
+#else
const char *argv[] = { "/bin/sh", "-c", command, NULL };
+#endif
trace_migration_exec_incoming(command);
ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
--
2.39.0
Hi On Mon, Jan 16, 2023 at 5:35 AM John Berberian, Jr <jeb.study@gmail.com> wrote: > > * Use cmd instead of /bin/sh on Windows. > > * Try to auto-detect cmd.exe's path, but default to a hard-coded path. > > Note that this will require that gspawn-win[32|64]-helper.exe and > gspawn-win[32|64]-helper-console.exe are included in the Windows binary > distributions (cc: Stefan Weil). > > Signed-off-by: "John Berberian, Jr" <jeb.study@gmail.com> > --- > Whoops, forgot a header. Here's a revised patch. > > migration/exec.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/migration/exec.c b/migration/exec.c > index 375d2e1b54..38604d73a6 100644 > --- a/migration/exec.c > +++ b/migration/exec.c > @@ -23,12 +23,31 @@ > #include "migration.h" > #include "io/channel-command.h" > #include "trace.h" > +#include "qemu/cutils.h" > > +#ifdef WIN32 > +const char *exec_get_cmd_path(void); > +const char *exec_get_cmd_path(void) > +{ > + g_autofree char *detected_path = g_new(char, MAX_PATH); > + if (GetSystemDirectoryA(detected_path, MAX_PATH) == 0) { > + warn_report("Could not detect cmd.exe path, using default."); > + return "C:\\Windows\\System32\\cmd.exe"; > + } > + pstrcat(detected_path, MAX_PATH, "\\cmd.exe"); > + return g_steal_pointer(&detected_path); > +} > +#endif > > void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp) > { > QIOChannel *ioc; > + > +#ifdef WIN32 > + const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL }; > +#else > const char *argv[] = { "/bin/sh", "-c", command, NULL }; > +#endif It may be a better idea to use g_shell_parse_argv() instead. > > trace_migration_exec_outgoing(command); > ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv, > @@ -55,7 +74,12 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc, > void exec_start_incoming_migration(const char *command, Error **errp) > { > QIOChannel *ioc; > + > +#ifdef WIN32 > + const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL }; > +#else > const char *argv[] = { "/bin/sh", "-c", command, NULL }; > +#endif > > trace_migration_exec_incoming(command); > ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv, > -- > 2.39.0 > > -- Marc-André Lureau
On Mon, Jan 16, 2023 at 11:17:08AM +0400, Marc-André Lureau wrote: > Hi > > On Mon, Jan 16, 2023 at 5:35 AM John Berberian, Jr <jeb.study@gmail.com> wrote: > > > > * Use cmd instead of /bin/sh on Windows. > > > > * Try to auto-detect cmd.exe's path, but default to a hard-coded path. > > > > Note that this will require that gspawn-win[32|64]-helper.exe and > > gspawn-win[32|64]-helper-console.exe are included in the Windows binary > > distributions (cc: Stefan Weil). > > > > Signed-off-by: "John Berberian, Jr" <jeb.study@gmail.com> > > --- > > Whoops, forgot a header. Here's a revised patch. > > > > migration/exec.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/migration/exec.c b/migration/exec.c > > index 375d2e1b54..38604d73a6 100644 > > --- a/migration/exec.c > > +++ b/migration/exec.c > > @@ -23,12 +23,31 @@ > > #include "migration.h" > > #include "io/channel-command.h" > > #include "trace.h" > > +#include "qemu/cutils.h" > > > > +#ifdef WIN32 > > +const char *exec_get_cmd_path(void); > > +const char *exec_get_cmd_path(void) > > +{ > > + g_autofree char *detected_path = g_new(char, MAX_PATH); > > + if (GetSystemDirectoryA(detected_path, MAX_PATH) == 0) { > > + warn_report("Could not detect cmd.exe path, using default."); > > + return "C:\\Windows\\System32\\cmd.exe"; > > + } > > + pstrcat(detected_path, MAX_PATH, "\\cmd.exe"); > > + return g_steal_pointer(&detected_path); > > +} > > +#endif > > > > void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp) > > { > > QIOChannel *ioc; > > + > > +#ifdef WIN32 > > + const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL }; > > +#else > > const char *argv[] = { "/bin/sh", "-c", command, NULL }; > > +#endif > > It may be a better idea to use g_shell_parse_argv() instead. For non-Windows that would not be compatible though. eg if someone is currently using shell redirection or pipelined commands: migrate "exec:dd of=/foo 1>/dev/null 2>&1" When we introduce a new QAPI format for migration args though, I've suggested we drop support for passing exec via shell, and require an explicit argv[] array: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg01434.html For Windows since we don't have back compat to worry about, we can avoid passing via cmd.exe from the start. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Apologies for the late response, I was traveling most of yesterday. On 1/16/23 4:22 AM, Daniel P. Berrangé wrote: > When we introduce a new QAPI format for migration args though, I've > suggested we drop support for passing exec via shell, and require an > explicit argv[] array: > > https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg01434.html > > For Windows since we don't have back compat to worry about, we > can avoid passing via cmd.exe from the start. I think we should keep the behavior the same on all platforms. If such a change is to occur, it should happen at the same time on Windows and Unix-like systems. Platform-dependent ifdefs should be used to overcome platform-specific differences (e.g. the location of the shell), rather than give one platform entirely different functionality - otherwise we introduce needless confusion when someone accustomed to Linux tries to use an exec migration on Windows and it doesn't work the same way at all. Best regards, John Berberian, Jr.
Hi On Tue, Jan 17, 2023 at 9:07 PM John Berberian, Jr <jeb.study@gmail.com> wrote: > > Apologies for the late response, I was traveling most of yesterday. > > On 1/16/23 4:22 AM, Daniel P. Berrangé wrote: > > When we introduce a new QAPI format for migration args though, I've > > suggested we drop support for passing exec via shell, and require an > > explicit argv[] array: > > > > https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg01434.html > > > > For Windows since we don't have back compat to worry about, we > > can avoid passing via cmd.exe from the start. > > I think we should keep the behavior the same on all platforms. If such a > change is to occur, it should happen at the same time on Windows and > Unix-like systems. Platform-dependent ifdefs should be used to overcome > platform-specific differences (e.g. the location of the shell), rather > than give one platform entirely different functionality - otherwise we > introduce needless confusion when someone accustomed to Linux tries to > use an exec migration on Windows and it doesn't work the same way at all. I agree with Daniel, we should make the migrate/exec command take an argv[] (not run through the shell) and deprecate support for "exec:.." in QMP. The "exec:..." form support could later be moved to HMP... Tbh, allowing fork/exec from QEMU is not a great thing in the first place (although with GSpawn using posix_spawn on modern systems, that should help.. and win32 has a different API). Instead, QMP/HMP clients could handle consumer process creation, and passing FDs via 'getfd,' and using the migrate 'fd:fdname' form (that is not really possible on win32 today, but I am adding support for importing sockets in a series on the list. This should do the job now that win32 supports unix sockets. We could also add support for pipes for older windows, and other kind of handles too). I admit this is not as convenient as the current "exec:cmdline" form... I don't know whether we have enough motivation to push those changes... I see it fitting with the goal to make HMP a human-friendly QMP client though. In the meantime, I guess we should take the proposed patch. Stefan, as win32 maintainer, any opinion? -- Marc-André Lureau
On Tue, Jan 31, 2023 at 02:01:07PM +0400, Marc-André Lureau wrote: > Hi > > On Tue, Jan 17, 2023 at 9:07 PM John Berberian, Jr <jeb.study@gmail.com> wrote: > > > > Apologies for the late response, I was traveling most of yesterday. > > > > On 1/16/23 4:22 AM, Daniel P. Berrangé wrote: > > > When we introduce a new QAPI format for migration args though, I've > > > suggested we drop support for passing exec via shell, and require an > > > explicit argv[] array: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg01434.html > > > > > > For Windows since we don't have back compat to worry about, we > > > can avoid passing via cmd.exe from the start. > > > > I think we should keep the behavior the same on all platforms. If such a > > change is to occur, it should happen at the same time on Windows and > > Unix-like systems. Platform-dependent ifdefs should be used to overcome > > platform-specific differences (e.g. the location of the shell), rather > > than give one platform entirely different functionality - otherwise we > > introduce needless confusion when someone accustomed to Linux tries to > > use an exec migration on Windows and it doesn't work the same way at all. > > I agree with Daniel, we should make the migrate/exec command take an > argv[] (not run through the shell) and deprecate support for "exec:.." > in QMP. The "exec:..." form support could later be moved to HMP... Note, I was *not* arguing in favour of deprecating 'exec:' support, only that we should prefer argv[] to bypass use of shell, because shell introducing massive scope for unintended consquences possibly with security implications. > Tbh, allowing fork/exec from QEMU is not a great thing in the first > place (although with GSpawn using posix_spawn on modern systems, that > should help.. and win32 has a different API). > > Instead, QMP/HMP clients could handle consumer process creation, and > passing FDs via 'getfd,' and using the migrate 'fd:fdname' form (that > is not really possible on win32 today, but I am adding support for > importing sockets in a series on the list. This should do the job now > that win32 supports unix sockets. We could also add support for pipes > for older windows, and other kind of handles too). I admit this is not > as convenient as the current "exec:cmdline" form... I don't know > whether we have enough motivation to push those changes... I see it > fitting with the goal to make HMP a human-friendly QMP client though. We could make the same argument against supporting the other migration protocols too, because all can be handled by merely passing in a pre-opened FD from outside. That is not very friendly though, even for QMP clients. I think it is sensible to have an exec: protocol in general as long as we bypass shell. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
ping. Is there anything I can do to help this get merged? Best regards, John Berberian, Jr.
"John Berberian, Jr" <jeb.study@gmail.com> wrote: > ping. Is there anything I can do to help this get merged? Hi I have to get back from Marc/Daniel before proceed. You did an answer, but they didn't respond. What should we do here? Thanks, Juan. > Best regards, > John Berberian, Jr.
Juan Quintela <quintela@redhat.com> wrote: > "John Berberian, Jr" <jeb.study@gmail.com> wrote: >> ping. Is there anything I can do to help this get merged? > > Hi > > I have to get back from Marc/Daniel before proceed. > > You did an answer, but they didn't respond. > > What should we do here? > > Thanks, Juan. Reviewed-by: Juan Quintela <quintela@redhat.com> queued. Althought I would have preffer to create a os_get_cmd_path() in both os-win32.c and os-posix.c, so we don't have the #ifdefs at all. If you consider doing that, let me know and I will drop this one. Later, Juan.
On Tue, Feb 28, 2023 at 12:35:02PM +0100, Juan Quintela wrote: > Juan Quintela <quintela@redhat.com> wrote: > > "John Berberian, Jr" <jeb.study@gmail.com> wrote: > >> ping. Is there anything I can do to help this get merged? > > > > Hi > > > > I have to get back from Marc/Daniel before proceed. > > > > You did an answer, but they didn't respond. > > > > What should we do here? > > > > Thanks, Juan. > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > queued. > > Althought I would have preffer to create a > > os_get_cmd_path() > > in both os-win32.c and os-posix.c, so we don't have the #ifdefs at all. IMHO it is preferable to NOT have it in os-posix/win32 as we don't want any other areas of QEMU to mistakenly think it is a good idea to use. This will be relatively short lived once we introduce the new migration parameters to replace the URI, and can deprecate the use of shell. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> wrote: > On Tue, Feb 28, 2023 at 12:35:02PM +0100, Juan Quintela wrote: >> Juan Quintela <quintela@redhat.com> wrote: >> > "John Berberian, Jr" <jeb.study@gmail.com> wrote: >> >> ping. Is there anything I can do to help this get merged? >> > >> > Hi >> > >> > I have to get back from Marc/Daniel before proceed. >> > >> > You did an answer, but they didn't respond. >> > >> > What should we do here? >> > >> > Thanks, Juan. >> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> >> queued. >> >> Althought I would have preffer to create a >> >> os_get_cmd_path() >> >> in both os-win32.c and os-posix.c, so we don't have the #ifdefs at all. > > IMHO it is preferable to NOT have it in os-posix/win32 as we don't > want any other areas of QEMU to mistakenly think it is a good idea > to use. This will be relatively short lived once we introduce the > new migration parameters to replace the URI, and can deprecate > the use of shell. ok, ok...... I will tell myself that it is just temporary O:-) Later, Juan.
On Tue, Feb 28, 2023 at 12:11:05PM +0100, Juan Quintela wrote: > "John Berberian, Jr" <jeb.study@gmail.com> wrote: > > ping. Is there anything I can do to help this get merged? > > Hi > > I have to get back from Marc/Daniel before proceed. > > You did an answer, but they didn't respond. > > What should we do here? I was hoping the patches that introduce the new migration QMP syntax would get in first, but its looking like it won't be so quick. So I'm OK introducing this patch in the meantime as a stop gap. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2024 Red Hat, Inc.