Patchwork [1/2] qga: distinguish binary modes in "guest_file_open_modes" map

login
register
mail settings
Submitter Laszlo Ersek
Date May 7, 2013, 4:56 p.m.
Message ID <1367945808-19979-1-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/242415/
State New
Headers show

Comments

Laszlo Ersek - May 7, 2013, 4:56 p.m.
In Windows guests this may make a difference.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)
Eric Blake - May 7, 2013, 5:19 p.m.
On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
> In Windows guests this may make a difference.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake - May 7, 2013, 5:27 p.m.
On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
> In Windows guests this may make a difference.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 04c6951..2eec712 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c

Oh, and only NOW do I notice that this is in a file named
commands-posix.c that doesn't get compiled into the Windows build of qga
(there, we only build commands-win32.c, and THAT file always fails this
command, because no one has ported guest-file-open there yet).  But I
guess there is still the argument that some weirdnix system exists that
isn't quite POSIX compliant and does have a distinct binary mode (maybe
someone plans on compiling qga for Cygwin instead of native windows,
since at least that would be able to open files when the
commands-win32.c variant doesn't?), so I still think the patch is worth
keeping.
Peter Maydell - May 7, 2013, 6:54 p.m.
On 7 May 2013 18:27, Eric Blake <eblake@redhat.com> wrote:
> On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
>> In Windows guests this may make a difference.
>>
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

> Oh, and only NOW do I notice that this is in a file named
> commands-posix.c that doesn't get compiled into the Windows build of qga
> (there, we only build commands-win32.c, and THAT file always fails this
> command, because no one has ported guest-file-open there yet).  But I
> guess there is still the argument that some weirdnix system exists that
> isn't quite POSIX compliant and does have a distinct binary mode (maybe
> someone plans on compiling qga for Cygwin instead of native windows,
> since at least that would be able to open files when the
> commands-win32.c variant doesn't?), so I still think the patch is worth
> keeping.

If we accept this we should at a minimum update the
commit message to say that we're doing this for
theoretical completeness rather than any practical reason.
(Not that "this may make a difference" is a particularly
convincing commit message in the first place :-))

thanks
-- PMM
Michael Roth - May 7, 2013, 8:10 p.m.
On Tue, May 07, 2013 at 11:27:03AM -0600, Eric Blake wrote:
> On 05/07/2013 10:56 AM, Laszlo Ersek wrote:
> > In Windows guests this may make a difference.
> > 
> > Suggested-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  qga/commands-posix.c |   22 ++++++++++++++++------
> >  1 files changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 04c6951..2eec712 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> 
> Oh, and only NOW do I notice that this is in a file named
> commands-posix.c that doesn't get compiled into the Windows build of qga
> (there, we only build commands-win32.c, and THAT file always fails this
> command, because no one has ported guest-file-open there yet).  But I
> guess there is still the argument that some weirdnix system exists that
> isn't quite POSIX compliant and does have a distinct binary mode (maybe
> someone plans on compiling qga for Cygwin instead of native windows,
> since at least that would be able to open files when the
> commands-win32.c variant doesn't?), so I still think the patch is worth
> keeping.

FWIW, I have some rough patches for w32 implementations of guest-file-*
commands queued up that I'll be cleaning up for 1.6, so it's not so much
theoretical as just a tad early :)

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 04c6951..2eec712 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -242,17 +242,27 @@  static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
 
 typedef const char * const ccpc;
 
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
+
 /* http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html */
 static const struct {
     ccpc *forms;
     int oflag_base;
 } guest_file_open_modes[] = {
-    { (ccpc[]){ "r",  "rb",         NULL }, O_RDONLY                      },
-    { (ccpc[]){ "w",  "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  },
-    { (ccpc[]){ "a",  "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND },
-    { (ccpc[]){ "r+", "rb+", "r+b", NULL }, O_RDWR                        },
-    { (ccpc[]){ "w+", "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  },
-    { (ccpc[]){ "a+", "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND }
+    { (ccpc[]){ "r",          NULL }, O_RDONLY                                 },
+    { (ccpc[]){ "rb",         NULL }, O_RDONLY                      | O_BINARY },
+    { (ccpc[]){ "w",          NULL }, O_WRONLY | O_CREAT | O_TRUNC             },
+    { (ccpc[]){ "wb",         NULL }, O_WRONLY | O_CREAT | O_TRUNC  | O_BINARY },
+    { (ccpc[]){ "a",          NULL }, O_WRONLY | O_CREAT | O_APPEND            },
+    { (ccpc[]){ "ab",         NULL }, O_WRONLY | O_CREAT | O_APPEND | O_BINARY },
+    { (ccpc[]){ "r+",         NULL }, O_RDWR                                   },
+    { (ccpc[]){ "rb+", "r+b", NULL }, O_RDWR                        | O_BINARY },
+    { (ccpc[]){ "w+",         NULL }, O_RDWR   | O_CREAT | O_TRUNC             },
+    { (ccpc[]){ "wb+", "w+b", NULL }, O_RDWR   | O_CREAT | O_TRUNC  | O_BINARY },
+    { (ccpc[]){ "a+",         NULL }, O_RDWR   | O_CREAT | O_APPEND            },
+    { (ccpc[]){ "ab+", "a+b", NULL }, O_RDWR   | O_CREAT | O_APPEND | O_BINARY }
 };
 
 static int