| Message ID | 1447105745-32358-1-git-send-email-kallan@suse.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
>>> > > 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
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 >
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
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 >
>>> > 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 >>
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)
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(-)