[Qemu-devel] [PATCH] qga: fix append file open modes for win32

[Qemu-devel] [PATCH] qga: fix append file open modes for win32

[Kirk Allan]

For append file open modes, use FILE_APPEND_DATA for the desired access for writing at the end of the file.

Signed-off-by: Kirk Allan <kallan@suse.com>
---
 qga/commands-win32.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index a5306e7..0a23b9b 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -70,16 +70,16 @@ static OpenFlags guest_file_open_modes[] = {
     {"rb",  GENERIC_READ,               OPEN_EXISTING},
     {"w",   GENERIC_WRITE,              CREATE_ALWAYS},
     {"wb",  GENERIC_WRITE,              CREATE_ALWAYS},
-    {"a",   GENERIC_WRITE,              OPEN_ALWAYS  },
+    {"a",   FILE_APPEND_DATA,           OPEN_ALWAYS  },
     {"r+",  GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
     {"rb+", GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
     {"r+b", GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
     {"w+",  GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
     {"wb+", GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
     {"w+b", GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
-    {"a+",  GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS  },
-    {"ab+", GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS  },
-    {"a+b", GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS  }
+    {"a+",  FILE_APPEND_DATA,           OPEN_ALWAYS  },
+    {"ab+", FILE_APPEND_DATA,           OPEN_ALWAYS  },
+    {"a+b", FILE_APPEND_DATA,           OPEN_ALWAYS  }
 };
 
 static OpenFlags *find_open_flag(const char *mode_str)
-- 
1.8.5.6

Re: [Qemu-devel] [PATCH] qga: fix append file open modes for win32

[Michael Roth]

Quoting Kirk Allan (2015-11-09 15:49:05)
> For append file open modes, use FILE_APPEND_DATA for the desired access for writing at the end of the file.
> 
> Signed-off-by: Kirk Allan <kallan@suse.com>
> ---
>  qga/commands-win32.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index a5306e7..0a23b9b 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -70,16 +70,16 @@ static OpenFlags guest_file_open_modes[] = {
>      {"rb",  GENERIC_READ,               OPEN_EXISTING},
>      {"w",   GENERIC_WRITE,              CREATE_ALWAYS},
>      {"wb",  GENERIC_WRITE,              CREATE_ALWAYS},
> -    {"a",   GENERIC_WRITE,              OPEN_ALWAYS  },
> +    {"a",   FILE_APPEND_DATA,           OPEN_ALWAYS  },
>      {"r+",  GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
>      {"rb+", GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
>      {"r+b", GENERIC_WRITE|GENERIC_READ, OPEN_EXISTING},
>      {"w+",  GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
>      {"wb+", GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
>      {"w+b", GENERIC_WRITE|GENERIC_READ, CREATE_ALWAYS},
> -    {"a+",  GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS  },
> -    {"ab+", GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS  },
> -    {"a+b", GENERIC_WRITE|GENERIC_READ, OPEN_ALWAYS  }
> +    {"a+",  FILE_APPEND_DATA,           OPEN_ALWAYS  },
> +    {"ab+", FILE_APPEND_DATA,           OPEN_ALWAYS  },
> +    {"a+b", FILE_APPEND_DATA,           OPEN_ALWAYS  }

Cc'ing qemu-stable@nongnu.org

Thanks for catching this, accidentally truncating files is
certainly not good...

I hit an issue testing this though, this does fix the append
case, but a+, ab+, a+b all imply append+read, while
FILE_APPEND_DATA only grants append access.

FILE_APPEND_DATA|GENERIC_READ seems to work, but I'm not
finding much official documentation on what's valid with
FILE_APPEND_DATA. Do you have a reference that might
confirm this is valid usage? The only reference to
FILE_APPEND_DATA I saw was a single comment in:

https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx

I'd like to get this in soon for 2.5 (hard freeze / rc0 is thursday).

>  };
> 
>  static OpenFlags *find_open_flag(const char *mode_str)
> -- 
> 1.8.5.6
> 

Re: [Qemu-devel] [PATCH] qga: fix append file open modes for win32

[Paolo Bonzini]

On 10/11/2015 16:40, Michael Roth wrote:
> 
> I hit an issue testing this though, this does fix the append
> case, but a+, ab+, a+b all imply append+read, while
> FILE_APPEND_DATA only grants append access.
> 
> FILE_APPEND_DATA|GENERIC_READ seems to work, but I'm not
> finding much official documentation on what's valid with
> FILE_APPEND_DATA. Do you have a reference that might
> confirm this is valid usage? The only reference to
> FILE_APPEND_DATA I saw was a single comment in:

I found a few references to FILE_APPEND_DATA, but not to combining it
with GENERIC_READ.

https://msdn.microsoft.com/en-us/library/windows/desktop/gg258116%28v=vs.85%29.aspx
(File Access Rights Constants) under FILE_APPEND_DATA says "For a file
object, the right to append data to the file. (For local files, write
operations will not overwrite existing data if this flag is specified
without FILE_WRITE_DATA.)".

https://msdn.microsoft.com/en-us/library/ff548289.aspx also says:

* If only the FILE_APPEND_DATA and SYNCHRONIZE flags are set, the caller
can write only to the end of the file, and any offset information about
writes to the file is ignored. However, the file will automatically be
extended as necessary for this type of write operation.

* Setting the FILE_WRITE_DATA flag for a file also allows writes beyond
the end of the file to occur. The file is automatically extended for
this type of write, as well.


Regarding the combination of reading and appending, GENERIC_READ and
GENERIC_WRITE are sort of "macro" access rights, which expand to
different attributes depending on what you're opening.  For files,
FILE_WRITE_DATA and FILE_READ_DATA are part of GENERIC_READ and
GENERIC_WRITE:

GENERIC_READ for files
	= FILE_READ_DATA
	+ FILE_READ_ATTRIBUTES
	+ FILE_READ_EA
	+ SYNCHRONIZE
	+ STANDARD_RIGHTS_READ (which is READ_CONTROL)

GENERIC_WRITE for files
	= FILE_APPEND_DATA
	+ FILE_WRITE_DATA
	+ FILE_WRITE_ATTRIBUTES
	+ FILE_WRITE_EA
	+ SYNCHRONIZE
	+ STANDARD_RIGHTS_WRITE (which is READ_CONTROL too!)

Of these of course qemu-ga only needs FILE_*_DATA and possibly SYNCHRONIZE.

The above description doesn't say what happens if you specify
FILE_READ_DATA and FILE_APPEND_DATA together, but I guess you can expect
it to just work.

Paolo

Re: [Qemu-devel] [PATCH] qga: fix append file open modes for win32

[Kirk Allan]

 >>>

> 
> On 10/11/2015 16:40, Michael Roth wrote:
>> 
>> I hit an issue testing this though, this does fix the append
>> case, but a+, ab+, a+b all imply append+read, while
>> FILE_APPEND_DATA only grants append access.
>> 
>> FILE_APPEND_DATA|GENERIC_READ seems to work, but I'm not
>> finding much official documentation on what's valid with
>> FILE_APPEND_DATA. Do you have a reference that might
>> confirm this is valid usage? The only reference to
>> FILE_APPEND_DATA I saw was a single comment in:
> 
> I found a few references to FILE_APPEND_DATA, but not to combining it
> with GENERIC_READ.
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/gg258116%28v=vs.85%
> 29.aspx
> (File Access Rights Constants) under FILE_APPEND_DATA says "For a file
> object, the right to append data to the file. (For local files, write
> operations will not overwrite existing data if this flag is specified
> without FILE_WRITE_DATA.)".
> 
> https://msdn.microsoft.com/en-us/library/ff548289.aspx also says:
> 
> * If only the FILE_APPEND_DATA and SYNCHRONIZE flags are set, the caller
> can write only to the end of the file, and any offset information about
> writes to the file is ignored. However, the file will automatically be
> extended as necessary for this type of write operation.
> 
> * Setting the FILE_WRITE_DATA flag for a file also allows writes beyond
> the end of the file to occur. The file is automatically extended for
> this type of write, as well.
> 
> 
> Regarding the combination of reading and appending, GENERIC_READ and
> GENERIC_WRITE are sort of "macro" access rights, which expand to
> different attributes depending on what you're opening.  For files,
> FILE_WRITE_DATA and FILE_READ_DATA are part of GENERIC_READ and
> GENERIC_WRITE:
> 
> GENERIC_READ for files
> 	= FILE_READ_DATA
> 	+ FILE_READ_ATTRIBUTES
> 	+ FILE_READ_EA
> 	+ SYNCHRONIZE
> 	+ STANDARD_RIGHTS_READ (which is READ_CONTROL)
> 
> GENERIC_WRITE for files
> 	= FILE_APPEND_DATA
> 	+ FILE_WRITE_DATA
> 	+ FILE_WRITE_ATTRIBUTES
> 	+ FILE_WRITE_EA
> 	+ SYNCHRONIZE
> 	+ STANDARD_RIGHTS_WRITE (which is READ_CONTROL too!)
> 
> Of these of course qemu-ga only needs FILE_*_DATA and possibly SYNCHRONIZE.
> 
> The above description doesn't say what happens if you specify
> FILE_READ_DATA and FILE_APPEND_DATA together, but I guess you can expect
> it to just work.

I haven't found any additional references.  I will test various combinations to see if any will produce the expected behavior.

- Kirk

> 
> Paolo

Re: [Qemu-devel] [PATCH] qga: fix append file open modes for win32

[Michael Roth]

Quoting Paolo Bonzini (2015-11-10 11:59:18)
> 
> 
> On 10/11/2015 16:40, Michael Roth wrote:
> > 
> > I hit an issue testing this though, this does fix the append
> > case, but a+, ab+, a+b all imply append+read, while
> > FILE_APPEND_DATA only grants append access.
> > 
> > FILE_APPEND_DATA|GENERIC_READ seems to work, but I'm not
> > finding much official documentation on what's valid with
> > FILE_APPEND_DATA. Do you have a reference that might
> > confirm this is valid usage? The only reference to
> > FILE_APPEND_DATA I saw was a single comment in:
> 
> I found a few references to FILE_APPEND_DATA, but not to combining it
> with GENERIC_READ.
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/gg258116%28v=vs.85%29.aspx
> (File Access Rights Constants) under FILE_APPEND_DATA says "For a file
> object, the right to append data to the file. (For local files, write
> operations will not overwrite existing data if this flag is specified
> without FILE_WRITE_DATA.)".
> 
> https://msdn.microsoft.com/en-us/library/ff548289.aspx also says:
> 
> * If only the FILE_APPEND_DATA and SYNCHRONIZE flags are set, the caller
> can write only to the end of the file, and any offset information about
> writes to the file is ignored. However, the file will automatically be
> extended as necessary for this type of write operation.
> 
> * Setting the FILE_WRITE_DATA flag for a file also allows writes beyond
> the end of the file to occur. The file is automatically extended for
> this type of write, as well.
> 
> 
> Regarding the combination of reading and appending, GENERIC_READ and
> GENERIC_WRITE are sort of "macro" access rights, which expand to
> different attributes depending on what you're opening.  For files,
> FILE_WRITE_DATA and FILE_READ_DATA are part of GENERIC_READ and
> GENERIC_WRITE:
> 
> GENERIC_READ for files
>         = FILE_READ_DATA
>         + FILE_READ_ATTRIBUTES
>         + FILE_READ_EA
>         + SYNCHRONIZE
>         + STANDARD_RIGHTS_READ (which is READ_CONTROL)
> 
> GENERIC_WRITE for files
>         = FILE_APPEND_DATA
>         + FILE_WRITE_DATA
>         + FILE_WRITE_ATTRIBUTES
>         + FILE_WRITE_EA
>         + SYNCHRONIZE
>         + STANDARD_RIGHTS_WRITE (which is READ_CONTROL too!)
> 
> Of these of course qemu-ga only needs FILE_*_DATA and possibly SYNCHRONIZE.
> 
> The above description doesn't say what happens if you specify
> FILE_READ_DATA and FILE_APPEND_DATA together, but I guess you can expect
> it to just work.

Thanks, this is very helpful. This seems to coincide with
FILE_GENERIC_WRITE / FILE_GENERIC_READ if you want to access the
bitmasks directly (though it looks like you can still add flags
to GENERIC_WRITE / GENERIC_READ):

  https://msdn.microsoft.com/en-us/library/windows/desktop/aa364399(v=vs.85).aspx

Looks like the crux of it is that FILE_WRITE_DATA causes us not to ignore
the start offset (0) for writes, so we end up writing data at the
beginning of the file instead of the end.

So I think the following
should work:

  a: FILE_GENERIC_WRITE & ~FILE_WRITE_DATA
  a+: FILE_GENERIC_READ | FILE_APPEND_DATA

FILE_APPEND_DATA by itself might work for a:, but for consistency I
think I'd prefer sticking with the generic set and masking out
FILE_WRITE_DATA.

> 
> Paolo
> 

Re: [Qemu-devel] [PATCH] qga: fix append file open modes for win32

[Paolo Bonzini]

On 11/11/2015 15:02, Michael Roth wrote:
>> GENERIC_READ for files
>>         = FILE_READ_DATA
>>         + FILE_READ_ATTRIBUTES
>>         + FILE_READ_EA
>>         + SYNCHRONIZE
>>         + STANDARD_RIGHTS_READ (which is READ_CONTROL)
>>
>> GENERIC_WRITE for files
>>         = FILE_APPEND_DATA
>>         + FILE_WRITE_DATA
>>         + FILE_WRITE_ATTRIBUTES
>>         + FILE_WRITE_EA
>>         + SYNCHRONIZE
>>         + STANDARD_RIGHTS_WRITE (which is READ_CONTROL too!)
>>
>> Of these of course qemu-ga only needs FILE_*_DATA and possibly SYNCHRONIZE.
>>
>> The above description doesn't say what happens if you specify
>> FILE_READ_DATA and FILE_APPEND_DATA together, but I guess you can expect
>> it to just work.
> 
> Thanks, this is very helpful. This seems to coincide with
> FILE_GENERIC_WRITE / FILE_GENERIC_READ if you want to access the
> bitmasks directly (though it looks like you can still add flags
> to GENERIC_WRITE / GENERIC_READ):
> 
>   https://msdn.microsoft.com/en-us/library/windows/desktop/aa364399(v=vs.85).aspx

Yes, I had missed the FILE_GENERIC_* definitions.  I found them now in
mingw as well, and they are exactly what you would expect (an | of the
various flags).

> Looks like the crux of it is that FILE_WRITE_DATA causes us not to ignore
> the start offset (0) for writes, so we end up writing data at the
> beginning of the file instead of the end.
> 
> So I think the following
> should work:
> 
>   a: FILE_GENERIC_WRITE & ~FILE_WRITE_DATA
>   a+: FILE_GENERIC_READ | FILE_APPEND_DATA
> 
> FILE_APPEND_DATA by itself might work for a:, but for consistency I
> think I'd prefer sticking with the generic set and masking out
> FILE_WRITE_DATA.

For a+ I would use any of

	(FILE_GENERIC_READ | FILE_GENERIC_WRITE) & ~FILE_WRITE_DATA
	GENERIC_READ | (FILE_GENERIC_WRITE & ~FILE_WRITE_DATA)

Perhaps you can define this:

#define FILE_GENERIC_APPEND	(FILE_GENERIC_WRITE & ~FILE_WRITE_DATA)

and then use

	a: FILE_GENERIC_APPEND
	a+: GENERIC_READ|FILE_GENERIC_APPEND

or

	a: FILE_GENERIC_APPEND
	a+: FILE_GENERIC_READ|FILE_GENERIC_APPEND

?

Paolo

Re: [Qemu-devel] [PATCH] qga: fix append file open modes for win32

[Michael Roth]

Quoting Paolo Bonzini (2015-11-11 08:49:57)
> 
> 
> On 11/11/2015 15:02, Michael Roth wrote:
> >> GENERIC_READ for files
> >>         = FILE_READ_DATA
> >>         + FILE_READ_ATTRIBUTES
> >>         + FILE_READ_EA
> >>         + SYNCHRONIZE
> >>         + STANDARD_RIGHTS_READ (which is READ_CONTROL)
> >>
> >> GENERIC_WRITE for files
> >>         = FILE_APPEND_DATA
> >>         + FILE_WRITE_DATA
> >>         + FILE_WRITE_ATTRIBUTES
> >>         + FILE_WRITE_EA
> >>         + SYNCHRONIZE
> >>         + STANDARD_RIGHTS_WRITE (which is READ_CONTROL too!)
> >>
> >> Of these of course qemu-ga only needs FILE_*_DATA and possibly SYNCHRONIZE.
> >>
> >> The above description doesn't say what happens if you specify
> >> FILE_READ_DATA and FILE_APPEND_DATA together, but I guess you can expect
> >> it to just work.
> > 
> > Thanks, this is very helpful. This seems to coincide with
> > FILE_GENERIC_WRITE / FILE_GENERIC_READ if you want to access the
> > bitmasks directly (though it looks like you can still add flags
> > to GENERIC_WRITE / GENERIC_READ):
> > 
> >   https://msdn.microsoft.com/en-us/library/windows/desktop/aa364399(v=vs.85).aspx
> 
> Yes, I had missed the FILE_GENERIC_* definitions.  I found them now in
> mingw as well, and they are exactly what you would expect (an | of the
> various flags).
> 
> > Looks like the crux of it is that FILE_WRITE_DATA causes us not to ignore
> > the start offset (0) for writes, so we end up writing data at the
> > beginning of the file instead of the end.
> > 
> > So I think the following
> > should work:
> > 
> >   a: FILE_GENERIC_WRITE & ~FILE_WRITE_DATA
> >   a+: FILE_GENERIC_READ | FILE_APPEND_DATA
> > 
> > FILE_APPEND_DATA by itself might work for a:, but for consistency I
> > think I'd prefer sticking with the generic set and masking out
> > FILE_WRITE_DATA.
> 
> For a+ I would use any of
> 
>         (FILE_GENERIC_READ | FILE_GENERIC_WRITE) & ~FILE_WRITE_DATA
>         GENERIC_READ | (FILE_GENERIC_WRITE & ~FILE_WRITE_DATA)
> 
> Perhaps you can define this:
> 
> #define FILE_GENERIC_APPEND     (FILE_GENERIC_WRITE & ~FILE_WRITE_DATA)
> 
> and then use
> 
>         a: FILE_GENERIC_APPEND
>         a+: GENERIC_READ|FILE_GENERIC_APPEND
> 
> or
> 
>         a: FILE_GENERIC_APPEND
>         a+: FILE_GENERIC_READ|FILE_GENERIC_APPEND

Yah, both are more proper actually (I was relying on FILE_GENERIC_READ
already containing the other flags from FILE_GENERIC_WRITE, but that's
more likely to break in the future).

I think I prefer the former since it avoids confusion on GENERIC_READ vs.
FILE_GENERIC_READ differences.

Kirk, I'm planning on squashing this into your v2 series, so no need to
resubmit.

Thanks!

> 
> ?
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH] qga: fix append file open modes for win32

[Kirk Allan]

 >>>
> Quoting Paolo Bonzini (2015-11-11 08:49:57)
>> 
>> 
>> On 11/11/2015 15:02, Michael Roth wrote:
>> >> GENERIC_READ for files
>> >>         = FILE_READ_DATA
>> >>         + FILE_READ_ATTRIBUTES
>> >>         + FILE_READ_EA
>> >>         + SYNCHRONIZE
>> >>         + STANDARD_RIGHTS_READ (which is READ_CONTROL)
>> >>
>> >> GENERIC_WRITE for files
>> >>         = FILE_APPEND_DATA
>> >>         + FILE_WRITE_DATA
>> >>         + FILE_WRITE_ATTRIBUTES
>> >>         + FILE_WRITE_EA
>> >>         + SYNCHRONIZE
>> >>         + STANDARD_RIGHTS_WRITE (which is READ_CONTROL too!)
>> >>
>> >> Of these of course qemu-ga only needs FILE_*_DATA and possibly SYNCHRONIZE.
>> >>
>> >> The above description doesn't say what happens if you specify
>> >> FILE_READ_DATA and FILE_APPEND_DATA together, but I guess you can expect
>> >> it to just work.
>> > 
>> > Thanks, this is very helpful. This seems to coincide with
>> > FILE_GENERIC_WRITE / FILE_GENERIC_READ if you want to access the
>> > bitmasks directly (though it looks like you can still add flags
>> > to GENERIC_WRITE / GENERIC_READ):
>> > 
>> >   
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa364399(v=vs.85).as
> px
>> 
>> Yes, I had missed the FILE_GENERIC_* definitions.  I found them now in
>> mingw as well, and they are exactly what you would expect (an | of the
>> various flags).
>> 
>> > Looks like the crux of it is that FILE_WRITE_DATA causes us not to ignore
>> > the start offset (0) for writes, so we end up writing data at the
>> > beginning of the file instead of the end.
>> > 
>> > So I think the following
>> > should work:
>> > 
>> >   a: FILE_GENERIC_WRITE & ~FILE_WRITE_DATA
>> >   a+: FILE_GENERIC_READ | FILE_APPEND_DATA
>> > 
>> > FILE_APPEND_DATA by itself might work for a:, but for consistency I
>> > think I'd prefer sticking with the generic set and masking out
>> > FILE_WRITE_DATA.
>> 
>> For a+ I would use any of
>> 
>>         (FILE_GENERIC_READ | FILE_GENERIC_WRITE) & ~FILE_WRITE_DATA
>>         GENERIC_READ | (FILE_GENERIC_WRITE & ~FILE_WRITE_DATA)
>> 
>> Perhaps you can define this:
>> 
>> #define FILE_GENERIC_APPEND     (FILE_GENERIC_WRITE & ~FILE_WRITE_DATA)
>> 
>> and then use
>> 
>>         a: FILE_GENERIC_APPEND
>>         a+: GENERIC_READ|FILE_GENERIC_APPEND
>> 
>> or
>> 
>>         a: FILE_GENERIC_APPEND
>>         a+: FILE_GENERIC_READ|FILE_GENERIC_APPEND
> 
> Yah, both are more proper actually (I was relying on FILE_GENERIC_READ
> already containing the other flags from FILE_GENERIC_WRITE, but that's
> more likely to break in the future).
> 
> I think I prefer the former since it avoids confusion on GENERIC_READ vs.
> FILE_GENERIC_READ differences.
> 
> Kirk, I'm planning on squashing this into your v2 series, so no need to
> resubmit.

Sounds great.  Thanks
- Kirk

> 
> Thanks!
> 
>> 
>> ?
>> 
>> Paolo
>>