Re: [Qemu-devel] [PATCH] qemu-ga: generate missing stubs for fsfreeze

[Michael Roth]

On Tue, Apr 17, 2012 at 01:54:49PM -0300, Luiz Capitulino wrote:
> On Tue, 17 Apr 2012 10:15:03 -0500
> Michael Roth <address@hidden> wrote:
> 
> > On Tue, Apr 17, 2012 at 11:44:39AM -0300, Luiz Capitulino wrote:
> > > On Fri, 13 Apr 2012 21:07:36 -0500
> > > Michael Roth <address@hidden> wrote:
> > > 
> > > > When linux-specific commands (including guest-fsfreeze-*) were 
> > > > consolidated
> > > > under defined(__linux__), we forgot to account for the case where
> > > > defined(__linux__) && !defined(FIFREEZE). As a result stubs are no 
> > > > longer
> > > > being generated on linux hosts that don't have FIFREEZE support. Fix
> > > > this.
> > > > 
> > > > Signed-off-by: Michael Roth <address@hidden>
> > > 
> > > Looks good to me, but I'm honestly a bit confused with the number of 
> > > ifdefs we
> > > have in this file. I think it's possible to re-organize them, but maybe 
> > > it's
> > > best to move the linux specific stuff to its own file.
> > > 
> > 
> > Yah, it was actually pretty nice and organized prior to this patch.
> 
> I think there's room for improvement. Today we have:
> 
>     #if defined(__linux__)
>     
>     -> includes
>     
>     #if defined(__linux__) && defined(FIREEZE)
>     #defined CONFIG_FSFREEZE
>     #endif
>     #endif
>     
>     #if defined(__linux__)
>     static void reopen_fd_to_null(int fd);
>     #endif
>     
>     -> non linux-specific code
>     
>     #if defined(__linux__)
>     
>     #if defined(CONFIG_FSFREEZE)
>     
>     -> fsfreeze functions
>     
>     #endif CONFIG_FSFREEZE
>     
>     -> linux specific functions
>     
>     #else /* !defined(__linux__) */
>     
>     -> fsfree & suspend stubs
>     
>     #endif
> 
> 
> I think we could have:
> 
> 
>     -> non-linux code
> 
>     #if defined(__linux__)
> 
>     -> includes

Personally I'd prefer grouping #includes together. I don't really have an
objection doing it this way though, but it doesn't bother me too much as
is.

> 
>     static void reopen_fd_to_null(int fd)

I actually left reopen_fd_to_null() out of the linux-only grouping because
it should actually be used by some other functions that call fork()
(mainly: shutdown, which isn't linux-specific). There's a TODO I stuck in
there with the recent consolidation as a place-holder.

> 
>     -> linux specific functions
> 
>     #if defined(FIREEZE)
> 
>     -> fsfreeze functions
> 
>     #else /* ! defined(FIREEZE) */
> 
>     -> fsfreeze stubs
> 
>     #endif /* defined(FIREEZE) */

I assume you meant to add here an:

#else /* ! defined(__linux__) */

> 
>     -> suspend stubs

We'd also need to copy/paste the fsfreeze stubs here if we did it this
way, which is why I ended up retaining the CONFIG_FSFREEZE macro and
generating those stubs on !CONFIG_FSFREEZE, outside of the
[!]defined(__linux__) blocks. CONFIG_FSFREEZE does obfuscate things a
bit though, and is somewhat redundant when used inside the linux-specific
block, so I agree we should drop it.

> 
>     #endif /* defined(__linux__) */
>