[Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[m a]

This patch replaces O_SYNC with O_FSYNC. These two flags do the same 
thing, but only O_FSYNC is available in Mac OS 10.3 and under. It only 
replaces O_SYNC if it doesn't exist. This patch allows the file 
block-raw-posix.c to compile on Mac OS 10.3. This is my first time 
submitting a patch, so there might have been a few mistakes made.

--- block-raw-posix.c	Wed May 20 16:46:58 2009
+++ block-raw-posix (updated).c	Fri Jun 19 22:01:07 2009
@@ -73,6 +73,12 @@
  #define DEBUG_BLOCK_PRINT(formatCstr, args...)
  #endif

+// O_SYNC isn't available on Mac OS 10.3 and under
+// O_SYNC and O_FSYNC do the same thing
+#ifndef O_SYNC
+#define O_SYNC O_FSYNC
+#endif
+
  /* OS X does not have O_DSYNC */
  #ifndef O_DSYNC
  #define O_DSYNC O_SYNC

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Anthony Liguori]

Hi,

m a wrote:
> This patch replaces O_SYNC with O_FSYNC. These two flags do the same 
> thing, but only O_FSYNC is available in Mac OS 10.3 and under. It only 
> replaces O_SYNC if it doesn't exist. This patch allows the file 
> block-raw-posix.c to compile on Mac OS 10.3. This is my first time 
> submitting a patch, so there might have been a few mistakes made.

Please include a Signed-off-by line and use C-style comments instead of 
C++ style comments.  Otherwise, looks good.

Regards,

Anthony Liguori

> --- block-raw-posix.c    Wed May 20 16:46:58 2009
> +++ block-raw-posix (updated).c    Fri Jun 19 22:01:07 2009
> @@ -73,6 +73,12 @@
>  #define DEBUG_BLOCK_PRINT(formatCstr, args...)
>  #endif
>
> +// O_SYNC isn't available on Mac OS 10.3 and under
> +// O_SYNC and O_FSYNC do the same thing
> +#ifndef O_SYNC
> +#define O_SYNC O_FSYNC
> +#endif
> +
>  /* OS X does not have O_DSYNC */
>  #ifndef O_DSYNC
>  #define O_DSYNC O_SYNC
>
>
>
>

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Avi Kivity]

On 06/20/2009 05:31 PM, Anthony Liguori wrote:
> Please include a Signed-off-by line and use C-style comments instead 
> of C++ style comments.  Otherwise, looks good.

What do we have against C++ style comments?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[François Revol]

> On 06/20/2009 05:31 PM, Anthony Liguori wrote:
> > Please include a Signed-off-by line and use C-style comments
> > instead
> > of C++ style comments.  Otherwise, looks good.
>
> What do we have against C++ style comments?

They are not valid in C code :P

François.

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Avi Kivity]

On 06/20/2009 10:03 PM, François Revol wrote:
>> On 06/20/2009 05:31 PM, Anthony Liguori wrote:
>>      
>>> Please include a Signed-off-by line and use C-style comments
>>> instead
>>> of C++ style comments.  Otherwise, looks good.
>>>        
>> What do we have against C++ style comments?
>>      
>
> They are not valid in C code :P
>    

I think C99 allows them.  Too bad date-related jokes are forbidden.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Jamie Lokier]

François Revol wrote:
> > On 06/20/2009 05:31 PM, Anthony Liguori wrote:
> > > Please include a Signed-off-by line and use C-style comments
> > > instead
> > > of C++ style comments.  Otherwise, looks good.
> >
> > What do we have against C++ style comments?
> 
> They are not valid in C code :P

Wrong.

Valid in GNU C for about 14 years, and ANSI/ISO C for about 10 years...

I'll be surprised if any C compiler used on any open source operating
system, or any major commercial C compiler, doesn't accept them.

Of course I still forbid them in my code too - they look wrong :-)

-- Jamie

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Andreas Färber]

Am 21.06.2009 um 01:25 schrieb Jamie Lokier:

> François Revol wrote:
>>> On 06/20/2009 05:31 PM, Anthony Liguori wrote:
>>>> Please [...] use C-style comments instead
>>>> of C++ style comments. [...]
>>>
>>> What do we have against C++ style comments?
>>
>> They are not valid in C code :P
>
> Wrong.
>
> Valid in GNU C for about 14 years, and ANSI/ISO C for about 10  
> years...
>
> I'll be surprised if any C compiler used on any open source operating
> system, or any major commercial C compiler, doesn't accept them.

Gladly even ten-year-old GCC 2.95.x supports 'em. But even a recent  
GCC does not when compiled with -ansi, which is why system headers  
still mustn't use them.

Andreas

> Of course I still forbid them in my code too - they look wrong :-)
>
> -- Jamie

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Jamie Lokier]

Andreas Färber wrote:
> 
> Am 21.06.2009 um 01:25 schrieb Jamie Lokier:
> 
> >François Revol wrote:
> >>>On 06/20/2009 05:31 PM, Anthony Liguori wrote:
> >>>>Please [...] use C-style comments instead
> >>>>of C++ style comments. [...]
> >>>
> >>>What do we have against C++ style comments?
> >>
> >>They are not valid in C code :P
> >
> >Wrong.
> >
> >Valid in GNU C for about 14 years, and ANSI/ISO C for about 10  
> >years...
> >
> >I'll be surprised if any C compiler used on any open source operating
> >system, or any major commercial C compiler, doesn't accept them.
> 
> Gladly even ten-year-old GCC 2.95.x supports 'em. But even a recent  
> GCC does not when compiled with -ansi, which is why system headers  
> still mustn't use them.

This is wrong too.  They can be used in system headers.

"gcc -ansi -pedantic -Wall -Werror" accepts C99 comments in system
headers.

It just complains about them in your own code, since you asked it to. :-)

-- Jamie

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Andreas Färber]

Am 24.06.2009 um 20:25 schrieb Jamie Lokier:

> Andreas Färber wrote:
>>
>> Am 21.06.2009 um 01:25 schrieb Jamie Lokier:
>>
>>> François Revol wrote:
>>>>> On 06/20/2009 05:31 PM, Anthony Liguori wrote:
>>>>>> Please [...] use C-style comments instead
>>>>>> of C++ style comments. [...]
>>>>>
>>>>> What do we have against C++ style comments?
>>>>
>>>> They are not valid in C code :P
>>>
>>> Wrong.
>>>
>>> Valid in GNU C for about 14 years, and ANSI/ISO C for about 10
>>> years...
>>>
>>> I'll be surprised if any C compiler used on any open source  
>>> operating
>>> system, or any major commercial C compiler, doesn't accept them.
>>
>> Gladly even ten-year-old GCC 2.95.x supports 'em. But even a recent
>> GCC does not when compiled with -ansi, which is why system headers
>> still mustn't use them.
>
> This is wrong too.  They can be used in system headers.

No, that is wrong. freetype2 definitely complained when Haiku had C99  
comments in its POSIX system headers. ;)

Andreas

> "gcc -ansi -pedantic -Wall -Werror" accepts C99 comments in system
> headers.
>
> It just complains about them in your own code, since you asked it  
> to. :-)
>
> -- Jamie

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Filip Navara]

On Wed, Jun 24, 2009 at 8:54 PM, Andreas Färber<andreas.faerber@web.de> wrote:
>
> Am 24.06.2009 um 20:25 schrieb Jamie Lokier:
>
>> Andreas Färber wrote:
>>>
>>> Am 21.06.2009 um 01:25 schrieb Jamie Lokier:
>>>
>>>> François Revol wrote:
>>>>>>
>>>>>> On 06/20/2009 05:31 PM, Anthony Liguori wrote:
>>>>>>>
>>>>>>> Please [...] use C-style comments instead
>>>>>>> of C++ style comments. [...]
>>>>>>
>>>>>> What do we have against C++ style comments?
>>>>>
>>>>> They are not valid in C code :P
>>>>
>>>> Wrong.
>>>>
>>>> Valid in GNU C for about 14 years, and ANSI/ISO C for about 10
>>>> years...
>>>>
>>>> I'll be surprised if any C compiler used on any open source operating
>>>> system, or any major commercial C compiler, doesn't accept them.
>>>
>>> Gladly even ten-year-old GCC 2.95.x supports 'em. But even a recent
>>> GCC does not when compiled with -ansi, which is why system headers
>>> still mustn't use them.
>>
>> This is wrong too.  They can be used in system headers.
>
> No, that is wrong. freetype2 definitely complained when Haiku had C99
> comments in its POSIX system headers. ;)

#pragma GCC system_header

Best regards,
Filip Navara

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Jamie Lokier]

Filip Navara wrote:
> On Wed, Jun 24, 2009 at 8:54 PM, Andreas Färber<andreas.faerber@web.de> wrote:
> >
> > Am 24.06.2009 um 20:25 schrieb Jamie Lokier:
> >
> >> Andreas Färber wrote:
> >>>
> >>> Am 21.06.2009 um 01:25 schrieb Jamie Lokier:
> >>>
> >>>> François Revol wrote:
> >>>>>>
> >>>>>> On 06/20/2009 05:31 PM, Anthony Liguori wrote:
> >>>>>>>
> >>>>>>> Please [...] use C-style comments instead
> >>>>>>> of C++ style comments. [...]
> >>>>>>
> >>>>>> What do we have against C++ style comments?
> >>>>>
> >>>>> They are not valid in C code :P
> >>>>
> >>>> Wrong.
> >>>>
> >>>> Valid in GNU C for about 14 years, and ANSI/ISO C for about 10
> >>>> years...
> >>>>
> >>>> I'll be surprised if any C compiler used on any open source operating
> >>>> system, or any major commercial C compiler, doesn't accept them.
> >>>
> >>> Gladly even ten-year-old GCC 2.95.x supports 'em. But even a recent
> >>> GCC does not when compiled with -ansi, which is why system headers
> >>> still mustn't use them.
> >>
> >> This is wrong too.  They can be used in system headers.
> >
> > No, that is wrong. freetype2 definitely complained when Haiku had C99
> > comments in its POSIX system headers. ;)
> 
> #pragma GCC system_header

GCC has a notion of certain directories being system header
directories.

So you don't need #pragma for headers in those, and if freetype2 was
installed in them, that should have worked.

But if you install those headers in other places, then it doesn't
treat them as system headers and becomes more strict about the
language.

You can add more system header directories, but of course (nearly)
everyone uses -Idir to add directories as that's well known and portable.

-- Jamie

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Anthony Liguori]

Avi Kivity wrote:
> On 06/20/2009 05:31 PM, Anthony Liguori wrote:
>> Please include a Signed-off-by line and use C-style comments instead 
>> of C++ style comments.  Otherwise, looks good.
>
> What do we have against C++ style comments?

Just consistency:

anthony@squirrel:~/git/qemu$ grep '//' *.c | wc -l
163
anthony@squirrel:~/git/qemu$ grep '/\*' *.c | wc -l
5642

Nothing personal.  I think the kernel has a policy of C89 comments only.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Avi Kivity]

On 06/20/2009 11:46 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 06/20/2009 05:31 PM, Anthony Liguori wrote:
>>> Please include a Signed-off-by line and use C-style comments instead 
>>> of C++ style comments.  Otherwise, looks good.
>>
>> What do we have against C++ style comments?
>
> Just consistency:
>
> anthony@squirrel:~/git/qemu$ grep '//' *.c | wc -l
> 163
> anthony@squirrel:~/git/qemu$ grep '/\*' *.c | wc -l
> 5642
>

Well, consistency trumps convenience.

> Nothing personal.  I think the kernel has a policy of C89 comments only.

This isn't the kernel, we have our own coding style.  But if we don't 
want C++ comments, that's fine.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Christoph Hellwig]

On Fri, Jun 19, 2009 at 10:22:07PM -0400, m a wrote:
> This patch replaces O_SYNC with O_FSYNC. These two flags do the same 
> thing, but only O_FSYNC is available in Mac OS 10.3 and under. It only 
> replaces O_SYNC if it doesn't exist. This patch allows the file 
> block-raw-posix.c to compile on Mac OS 10.3. This is my first time 
> submitting a patch, so there might have been a few mistakes made.

But O_SYNC is a standard posix flag, while O_FSYNC appears to be
a BSD extension.  Also the actual code uses O_DSYNC anyway, which
also is in Posix but not actually natively supported by some OSes,
e.g. Linux (but still provided in libc there).


> --- block-raw-posix.c	Wed May 20 16:46:58 2009
> +++ block-raw-posix (updated).c	Fri Jun 19 22:01:07 2009
> @@ -73,6 +73,12 @@
> 
> +// O_SYNC isn't available on Mac OS 10.3 and under
> +// O_SYNC and O_FSYNC do the same thing
> +#ifndef O_SYNC
> +#define O_SYNC O_FSYNC
> +#endif
> +
>  /* OS X does not have O_DSYNC */
>  #ifndef O_DSYNC
>  #define O_DSYNC O_SYNC

So if the code here is correct and Darwin is the only supported OS where
O_DSYNC is missing we could just replace the O_SYNC in the last line
with O_FSYNC.

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Jamie Lokier]

Christoph Hellwig wrote:
> On Fri, Jun 19, 2009 at 10:22:07PM -0400, m a wrote:
> > This patch replaces O_SYNC with O_FSYNC. These two flags do the same 
> > thing, but only O_FSYNC is available in Mac OS 10.3 and under. It only 
> > replaces O_SYNC if it doesn't exist. This patch allows the file 
> > block-raw-posix.c to compile on Mac OS 10.3. This is my first time 
> > submitting a patch, so there might have been a few mistakes made.
> 
> But O_SYNC is a standard posix flag, while O_FSYNC appears to be
> a BSD extension.  Also the actual code uses O_DSYNC anyway, which
> also is in Posix but not actually natively supported by some OSes,
> e.g. Linux (but still provided in libc there).

If O_FSYNC and O_SYNC do the same thing, and O_SYNC is used anywhere,
there's no harm in this for portability:

    #if !defined(O_SYNC) && defined(O_FSYNC)
    #define O_SYNC O_FSYNC
    #endif

The patch assumes O_FSYNC is defined if O_SYNC isn't, which is wrong.

> >  /* OS X does not have O_DSYNC */
> >  #ifndef O_DSYNC
> >  #define O_DSYNC O_SYNC
> 
> So if the code here is correct and Darwin is the only supported OS where
> O_DSYNC is missing we could just replace the O_SYNC in the last line
> with O_FSYNC.

I agree, though the comment might be misleading, if there's another
supported OS without O_DSYNC.

-- Jamie

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Anthony Liguori]

Jamie Lokier wrote:
> If O_FSYNC and O_SYNC do the same thing, and O_SYNC is used anywhere,
> there's no harm in this for portability:
>
>     #if !defined(O_SYNC) && defined(O_FSYNC)
>     #define O_SYNC O_FSYNC
>     #endif
>
> The patch assumes O_FSYNC is defined if O_SYNC isn't, which is wrong.
>   

For what this patch is, it really isn't that important IMHO.

This is just a work around for a broken platform.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[G 3]

On Jun 20, 2009, at 7:37 PM, Anthony Liguori wrote:

> Jamie Lokier wrote:
>> If O_FSYNC and O_SYNC do the same thing, and O_SYNC is used anywhere,
>> there's no harm in this for portability:
>>
>>     #if !defined(O_SYNC) && defined(O_FSYNC)
>>     #define O_SYNC O_FSYNC
>>     #endif
>>
>> The patch assumes O_FSYNC is defined if O_SYNC isn't, which is wrong.
>>
>
> For what this patch is, it really isn't that important IMHO.
>
> This is just a work around for a broken platform.
>

Even though you are correct, did you have to add the 'broken platform' 
remark?

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[Jamie Lokier]

G 3 wrote:
> Even though you are correct, did you have to add the 'broken platform' 
> remark?

Well, O_SYNC is POSIX so anything which claims to be a unix and
doesn't have O_SYNC is broken :-)

-- Jamie

Re: [Qemu-devel] [PATCH] replace O_SYNC with O_FSYNC

[G 3]

On Jun 20, 2009, at 7:30 PM, Jamie Lokier wrote:

> Christoph Hellwig wrote:
>> On Fri, Jun 19, 2009 at 10:22:07PM -0400, m a wrote:
>>> This patch replaces O_SYNC with O_FSYNC. These two flags do the same
>>> thing, but only O_FSYNC is available in Mac OS 10.3 and under. It 
>>> only
>>> replaces O_SYNC if it doesn't exist. This patch allows the file
>>> block-raw-posix.c to compile on Mac OS 10.3. This is my first time
>>> submitting a patch, so there might have been a few mistakes made.
>>
>> But O_SYNC is a standard posix flag, while O_FSYNC appears to be
>> a BSD extension.  Also the actual code uses O_DSYNC anyway, which
>> also is in Posix but not actually natively supported by some OSes,
>> e.g. Linux (but still provided in libc there).
>
> If O_FSYNC and O_SYNC do the same thing, and O_SYNC is used anywhere,
> there's no harm in this for portability:
>
>     #if !defined(O_SYNC) && defined(O_FSYNC)
>     #define O_SYNC O_FSYNC
>     #endif
>
> The patch assumes O_FSYNC is defined if O_SYNC isn't, which is wrong.
>
>>>  /* OS X does not have O_DSYNC */
>>>  #ifndef O_DSYNC
>>>  #define O_DSYNC O_SYNC
>>
>> So if the code here is correct and Darwin is the only supported OS 
>> where
>> O_DSYNC is missing we could just replace the O_SYNC in the last line
>> with O_FSYNC.
>
> I agree, though the comment might be misleading, if there's another
> supported OS without O_DSYNC.
>
> -- Jamie
>
>

Your logic does make sense. I guess I should have done this instead:

#ifndef O_SYNC
	#ifdef O_FSYNC
		#define O_SYNC 	O_FSYNC
	
	#else
		#define O_SYNC 	0
	
	#endif
#endif

Defining O_SYNC as zero is ok because this flag is 'ORed' with other 
flags.