diff mbox

Fixes a bug that tries to use the unimplemented function fdatasync on Mac OS X.

Message ID A428939A-2D4D-4F89-8C42-85AA500F3D6D@gmail.com
State Superseded
Headers show

Commit Message

Programmingkid Sept. 20, 2009, 12:58 a.m. UTC
This patch fixes a problem in the file cutils.c that prevents qemu  
from being built on Mac OS X. This patch makes sure fsync is used  
instead.

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>

---
  cutils.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Anthony Liguori Sept. 30, 2009, 7:16 p.m. UTC | #1
G 3 wrote:
> This patch fixes a problem in the file cutils.c that prevents qemu 
> from being built on Mac OS X. This patch makes sure fsync is used instead.

What is this against?  It doesn't apply to git.

> Signed-off-by: John Arbuckle <programmingkidx@gmail.com 
> <mailto:programmingkidx@gmail.com>>
>
> ---
>  cutils.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 7a22346..84eabf2 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -124,7 +124,7 @@ int qemu_fls(int i)
>   */
>  int qemu_fdatasync(int fd)
>  {
> -#ifdef _POSIX_SYNCHRONIZED_IO
> +#if ( defined(_POSIX_SYNCHRONIZED_IO) && !defined(__APPLE__) )
>      return fdatasync(fd);
>  #else
>      return fsync(fd);
> -- 
> 1.6.4.2
>
Programmingkid Oct. 1, 2009, 3:30 p.m. UTC | #2
On Sep 30, 2009, at 3:16 PM, Anthony Liguori wrote:

> G 3 wrote:
>> This patch fixes a problem in the file cutils.c that prevents qemu  
>> from being built on Mac OS X. This patch makes sure fsync is used  
>> instead.
>
> What is this against?  It doesn't apply to git.

This is against the savannah repository: git:// 
git.savannah.nongnu.org/qemu.git. Is this the wrong repository?
Blue Swirl Oct. 1, 2009, 4:13 p.m. UTC | #3
On Thu, Oct 1, 2009 at 6:30 PM, G 3 <programmingkidx@gmail.com> wrote:
> On Sep 30, 2009, at 3:16 PM, Anthony Liguori wrote:
>
> G 3 wrote:
>
> This patch fixes a problem in the file cutils.c that prevents qemu from
> being built on Mac OS X. This patch makes sure fsync is used instead.
>
> What is this against?  It doesn't apply to git.
>
> This is against the savannah
> repository: git://git.savannah.nongnu.org/qemu.git. Is this the wrong
> repository?

The patch should not be needed since
5f6b9e8fd5b9516170e582d9b6c27c98519a8031. Do you still have a problem?
Pierre Riteau Dec. 8, 2009, 2:35 p.m. UTC | #4
On 1 oct. 2009, at 18:13, Blue Swirl wrote:

> On Thu, Oct 1, 2009 at 6:30 PM, G 3 <programmingkidx@gmail.com> wrote:
>> On Sep 30, 2009, at 3:16 PM, Anthony Liguori wrote:
>> 
>> G 3 wrote:
>> 
>> This patch fixes a problem in the file cutils.c that prevents qemu from
>> being built on Mac OS X. This patch makes sure fsync is used instead.
>> 
>> What is this against?  It doesn't apply to git.
>> 
>> This is against the savannah
>> repository: git://git.savannah.nongnu.org/qemu.git. Is this the wrong
>> repository?
> 
> The patch should not be needed since
> 5f6b9e8fd5b9516170e582d9b6c27c98519a8031. Do you still have a problem?

Sorry to dig up this old thread, but I just tried compiling qemu on my Mac and I see a warning when it compiles:
cutils.c: In function ‘qemu_fdatasync’:
cutils.c:128: warning: implicit declaration of function ‘fdatasync’

I'm running OS X 10.6.2 with Xcode 3.2.1.

Why this behavior? Because the configure check introduced in 5f6b9e finds that fdatasync is available.
Although the fdatasync function is not referenced in the standard includes (hence the warning), it is present as a syscall:
/usr/include/sys/syscall.h:#define	SYS_fdatasync      187
Moreover, a fdatasync symbol is available through libSystem (which is why the linking done by the configure check works).

It's not clear what is the best solution here. Googling the issue, some projects add the fdatasync prototype to their headers to avoid warnings.
Other assume that fdatasync could be a noop and fall back to fsync.

I would go with the second second: the function is not defined, there is no man page, let's assume it doesn't work.
To fix the configure check we can simply add -Werror to the compile_prog cflags (I will send a patch if you agree with this).
Blue Swirl Dec. 8, 2009, 6:35 p.m. UTC | #5
On Tue, Dec 8, 2009 at 4:35 PM, Pierre Riteau <Pierre.Riteau@irisa.fr> wrote:
> On 1 oct. 2009, at 18:13, Blue Swirl wrote:
>
>> On Thu, Oct 1, 2009 at 6:30 PM, G 3 <programmingkidx@gmail.com> wrote:
>>> On Sep 30, 2009, at 3:16 PM, Anthony Liguori wrote:
>>>
>>> G 3 wrote:
>>>
>>> This patch fixes a problem in the file cutils.c that prevents qemu from
>>> being built on Mac OS X. This patch makes sure fsync is used instead.
>>>
>>> What is this against?  It doesn't apply to git.
>>>
>>> This is against the savannah
>>> repository: git://git.savannah.nongnu.org/qemu.git. Is this the wrong
>>> repository?
>>
>> The patch should not be needed since
>> 5f6b9e8fd5b9516170e582d9b6c27c98519a8031. Do you still have a problem?
>
> Sorry to dig up this old thread, but I just tried compiling qemu on my Mac and I see a warning when it compiles:
> cutils.c: In function ‘qemu_fdatasync’:
> cutils.c:128: warning: implicit declaration of function ‘fdatasync’
>
> I'm running OS X 10.6.2 with Xcode 3.2.1.
>
> Why this behavior? Because the configure check introduced in 5f6b9e finds that fdatasync is available.
> Although the fdatasync function is not referenced in the standard includes (hence the warning), it is present as a syscall:
> /usr/include/sys/syscall.h:#define      SYS_fdatasync      187
> Moreover, a fdatasync symbol is available through libSystem (which is why the linking done by the configure check works).
>
> It's not clear what is the best solution here. Googling the issue, some projects add the fdatasync prototype to their headers to avoid warnings.
> Other assume that fdatasync could be a noop and fall back to fsync.
>
> I would go with the second second: the function is not defined, there is no man page, let's assume it doesn't work.
> To fix the configure check we can simply add -Werror to the compile_prog cflags (I will send a patch if you agree with this).

I'd rather add something like
#ifdef __APPLE__
 exit(1);
#endif
(or the shell equivalent for configure) and a comment with your explanation.
Pierre Riteau Dec. 8, 2009, 9:02 p.m. UTC | #6
On 8 déc. 2009, at 19:35, Blue Swirl wrote:

> On Tue, Dec 8, 2009 at 4:35 PM, Pierre Riteau <Pierre.Riteau@irisa.fr> wrote:
>> On 1 oct. 2009, at 18:13, Blue Swirl wrote:
>> 
>>> On Thu, Oct 1, 2009 at 6:30 PM, G 3 <programmingkidx@gmail.com> wrote:
>>>> On Sep 30, 2009, at 3:16 PM, Anthony Liguori wrote:
>>>> 
>>>> G 3 wrote:
>>>> 
>>>> This patch fixes a problem in the file cutils.c that prevents qemu from
>>>> being built on Mac OS X. This patch makes sure fsync is used instead.
>>>> 
>>>> What is this against?  It doesn't apply to git.
>>>> 
>>>> This is against the savannah
>>>> repository: git://git.savannah.nongnu.org/qemu.git. Is this the wrong
>>>> repository?
>>> 
>>> The patch should not be needed since
>>> 5f6b9e8fd5b9516170e582d9b6c27c98519a8031. Do you still have a problem?
>> 
>> Sorry to dig up this old thread, but I just tried compiling qemu on my Mac and I see a warning when it compiles:
>> cutils.c: In function ‘qemu_fdatasync’:
>> cutils.c:128: warning: implicit declaration of function ‘fdatasync’
>> 
>> I'm running OS X 10.6.2 with Xcode 3.2.1.
>> 
>> Why this behavior? Because the configure check introduced in 5f6b9e finds that fdatasync is available.
>> Although the fdatasync function is not referenced in the standard includes (hence the warning), it is present as a syscall:
>> /usr/include/sys/syscall.h:#define      SYS_fdatasync      187
>> Moreover, a fdatasync symbol is available through libSystem (which is why the linking done by the configure check works).
>> 
>> It's not clear what is the best solution here. Googling the issue, some projects add the fdatasync prototype to their headers to avoid warnings.
>> Other assume that fdatasync could be a noop and fall back to fsync.
>> 
>> I would go with the second second: the function is not defined, there is no man page, let's assume it doesn't work.
>> To fix the configure check we can simply add -Werror to the compile_prog cflags (I will send a patch if you agree with this).
> 
> I'd rather add something like
> #ifdef __APPLE__
> exit(1);
> #endif
> (or the shell equivalent for configure) and a comment with your explanation.


I'm fine with that too.
Alexander Graf Dec. 8, 2009, 11:01 p.m. UTC | #7
Am 08.12.2009 um 19:35 schrieb Blue Swirl <blauwirbel@gmail.com>:

> On Tue, Dec 8, 2009 at 4:35 PM, Pierre Riteau  
> <Pierre.Riteau@irisa.fr> wrote:
>> On 1 oct. 2009, at 18:13, Blue Swirl wrote:
>>
>>> On Thu, Oct 1, 2009 at 6:30 PM, G 3 <programmingkidx@gmail.com>  
>>> wrote:
>>>> On Sep 30, 2009, at 3:16 PM, Anthony Liguori wrote:
>>>>
>>>> G 3 wrote:
>>>>
>>>> This patch fixes a problem in the file cutils.c that prevents  
>>>> qemu from
>>>> being built on Mac OS X. This patch makes sure fsync is used  
>>>> instead.
>>>>
>>>> What is this against?  It doesn't apply to git.
>>>>
>>>> This is against the savannah
>>>> repository: git://git.savannah.nongnu.org/qemu.git. Is this the  
>>>> wrong
>>>> repository?
>>>
>>> The patch should not be needed since
>>> 5f6b9e8fd5b9516170e582d9b6c27c98519a8031. Do you still have a  
>>> problem?
>>
>> Sorry to dig up this old thread, but I just tried compiling qemu on  
>> my Mac and I see a warning when it compiles:
>> cutils.c: In function ‘qemu_fdatasync’:
>> cutils.c:128: warning: implicit declaration of function ‘fdatasync’
>>
>> I'm running OS X 10.6.2 with Xcode 3.2.1.
>>
>> Why this behavior? Because the configure check introduced in 5f6b9e  
>> finds that fdatasync is available.
>> Although the fdatasync function is not referenced in the standard  
>> includes (hence the warning), it is present as a syscall:
>> /usr/include/sys/syscall.h:#define      SYS_fdatasync      187
>> Moreover, a fdatasync symbol is available through libSystem (which  
>> is why the linking done by the configure check works).
>>
>> It's not clear what is the best solution here. Googling the issue,  
>> some projects add the fdatasync prototype to their headers to avoid  
>> warnings.
>> Other assume that fdatasync could be a noop and fall back to fsync.
>>
>> I would go with the second second: the function is not defined,  
>> there is no man page, let's assume it doesn't work.
>> To fix the configure check we can simply add -Werror to the  
>> compile_prog cflags (I will send a patch if you agree with this).
>
> I'd rather add something like
> #ifdef __APPLE__
> exit(1);
> #endif
> (or the shell equivalent for configure) and a comment with your  
> explanation.
>
>


I guess that's wrong. With --disable-werror it compiles just fine. So  
I think we're rather missing a header include.

Alex
Pierre Riteau Dec. 10, 2009, 4:49 p.m. UTC | #8
On 9 déc. 2009, at 00:01, Alexander Graf wrote:

> 
> Am 08.12.2009 um 19:35 schrieb Blue Swirl <blauwirbel@gmail.com>:
> 
>> On Tue, Dec 8, 2009 at 4:35 PM, Pierre Riteau <Pierre.Riteau@irisa.fr> wrote:
>>> On 1 oct. 2009, at 18:13, Blue Swirl wrote:
>>> 
>>>> On Thu, Oct 1, 2009 at 6:30 PM, G 3 <programmingkidx@gmail.com> wrote:
>>>>> On Sep 30, 2009, at 3:16 PM, Anthony Liguori wrote:
>>>>> 
>>>>> G 3 wrote:
>>>>> 
>>>>> This patch fixes a problem in the file cutils.c that prevents qemu from
>>>>> being built on Mac OS X. This patch makes sure fsync is used instead.
>>>>> 
>>>>> What is this against?  It doesn't apply to git.
>>>>> 
>>>>> This is against the savannah
>>>>> repository: git://git.savannah.nongnu.org/qemu.git. Is this the wrong
>>>>> repository?
>>>> 
>>>> The patch should not be needed since
>>>> 5f6b9e8fd5b9516170e582d9b6c27c98519a8031. Do you still have a problem?
>>> 
>>> Sorry to dig up this old thread, but I just tried compiling qemu on my Mac and I see a warning when it compiles:
>>> cutils.c: In function ‘qemu_fdatasync’:
>>> cutils.c:128: warning: implicit declaration of function ‘fdatasync’
>>> 
>>> I'm running OS X 10.6.2 with Xcode 3.2.1.
>>> 
>>> Why this behavior? Because the configure check introduced in 5f6b9e finds that fdatasync is available.
>>> Although the fdatasync function is not referenced in the standard includes (hence the warning), it is present as a syscall:
>>> /usr/include/sys/syscall.h:#define      SYS_fdatasync      187
>>> Moreover, a fdatasync symbol is available through libSystem (which is why the linking done by the configure check works).
>>> 
>>> It's not clear what is the best solution here. Googling the issue, some projects add the fdatasync prototype to their headers to avoid warnings.
>>> Other assume that fdatasync could be a noop and fall back to fsync.
>>> 
>>> I would go with the second second: the function is not defined, there is no man page, let's assume it doesn't work.
>>> To fix the configure check we can simply add -Werror to the compile_prog cflags (I will send a patch if you agree with this).
>> 
>> I'd rather add something like
>> #ifdef __APPLE__
>> exit(1);
>> #endif
>> (or the shell equivalent for configure) and a comment with your explanation.
>> 
>> 
> 
> 
> I guess that's wrong. With --disable-werror it compiles just fine. So I think we're rather missing a header include.
> 
> Alex


Compiling just fine doesn't necessarily mean thatt qemu will run fine and do what we want it to do.
But in this case it may: i took a look at the XNU sources (I didn't think about this solution first, I forgot it was open source...) and fdatasync appears to be implemented.
So I guess that we can probably add the missing prototype in one of our header?
diff mbox

Patch

diff --git a/cutils.c b/cutils.c
index 7a22346..84eabf2 100644
--- a/cutils.c
+++ b/cutils.c
@@ -124,7 +124,7 @@  int qemu_fls(int i)
   */
  int qemu_fdatasync(int fd)
  {
-#ifdef _POSIX_SYNCHRONIZED_IO
+#if ( defined(_POSIX_SYNCHRONIZED_IO) && !defined(__APPLE__) )
      return fdatasync(fd);
  #else
      return fsync(fd);