[PATCH v2] Fix exec migration on Windows (w32+w64).

[PATCH v2] Fix exec migration on Windows (w32+w64).

[John Berberian, Jr]

* 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

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[Marc-André Lureau]

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

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[Daniel P. Berrangé]

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 :|

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[John Berberian, Jr]

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.

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[John Berberian, Jr.]

ping.
As requested in the wiki, here's the patchew link:
https://patchew.org/QEMU/20230116013421.3149183-1-jeb.study@gmail.com/

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[Marc-André Lureau]

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

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[John Berberian, Jr]

ping. Is there anything I can do to help this get merged?

Best regards,
John Berberian, Jr.

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[Juan Quintela]

"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.

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[Daniel P. Berrangé]

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 :|

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[Daniel P. Berrangé]

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 :|

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[Juan Quintela]

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.

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[Daniel P. Berrangé]

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 :|

Re: [PATCH v2] Fix exec migration on Windows (w32+w64).

[Juan Quintela]

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.