Patchwork qemu-ga: generate missing stubs for fsfreeze

login
register
mail settings
Submitter Michael Roth
Date April 14, 2012, 2:07 a.m.
Message ID <1334369256-32663-1-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/152469/
State New
Headers show

Comments

Michael Roth - April 14, 2012, 2:07 a.m.
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 <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c |   36 ++++++++++++++++++++----------------
 1 files changed, 20 insertions(+), 16 deletions(-)
Luiz Capitulino - April 17, 2012, 2:44 p.m.
On Fri, 13 Apr 2012 21:07:36 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com>

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.
Michael Roth - April 17, 2012, 3:15 p.m.
On Tue, Apr 17, 2012 at 11:44:39AM -0300, Luiz Capitulino wrote:
> On Fri, 13 Apr 2012 21:07:36 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com>
> 
> 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. Just 1
place/#ifdef __linux__ for determining whether to implement or stub. The
fsfreeze stuff is unfortunate because it's both linux-specific and
distro-dependent, so if we moved linux-specific commands to another file
we'd still need to include an #ifdef for fsfreeze. Plus I'd really like
to avoid doing anything that would encourage linux-specific
implementations. Needing to wrap your code in #ifdefs is punishment of sorts :P
Plus I think over time all the current RPCs other than fsfreeze can be
implemented for non-linux, so this is technically the right place for them.
Andreas Färber - April 17, 2012, 3:28 p.m.
Am 14.04.2012 04:07, schrieb Michael Roth:
> 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 <mdroth@linux.vnet.ibm.com>

Tested-by: Andreas Färber <afaerber@suse.de>

Thanks for the quick fix,

Andreas
Luiz Capitulino - April 17, 2012, 4:54 p.m.
On Tue, 17 Apr 2012 10:15:03 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com>
> > 
> > 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

    static void reopen_fd_to_null(int fd)

    -> linux specific functions

    #if defined(FIREEZE)

    -> fsfreeze functions

    #else /* ! defined(FIREEZE) */

    -> fsfreeze stubs

    #endif /* defined(FIREEZE) */

    -> suspend stubs

    #endif /* defined(__linux__) */
Michael Roth - April 17, 2012, 9:45 p.m.
On Tue, Apr 17, 2012 at 01:54:49PM -0300, Luiz Capitulino wrote:
> On Tue, 17 Apr 2012 10:15:03 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com>
> > > 
> > > 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__) */
>
Alexander Graf - April 19, 2012, 2:12 p.m.
On 04/17/2012 05:28 PM, Andreas Färber wrote:
> Am 14.04.2012 04:07, schrieb Michael Roth:
>> 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<mdroth@linux.vnet.ibm.com>
> Tested-by: Andreas Färber<afaerber@suse.de>

Any conclusions? Today's tree is still broken for me.


Alex
Michael Roth - April 19, 2012, 3:40 p.m.
On Thu, Apr 19, 2012 at 04:12:47PM +0200, Alexander Graf wrote:
> On 04/17/2012 05:28 PM, Andreas Färber wrote:
> >Am 14.04.2012 04:07, schrieb Michael Roth:
> >>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<mdroth@linux.vnet.ibm.com>
> >Tested-by: Andreas Färber<afaerber@suse.de>
> 
> Any conclusions? Today's tree is still broken for me.

I didn't have anything in my qemu-ga queue so I submitted through
qemu-trivial:

http://comments.gmane.org/gmane.comp.emulators.qemu/146895

Since it's a build fix it probably makes more sense to just send a Anthony a
small PULL though so I'll send that out shortly.

> 
> 
> Alex
> 
>
Alexander Graf - April 19, 2012, 3:42 p.m.
On 04/19/2012 05:40 PM, Michael Roth wrote:
> On Thu, Apr 19, 2012 at 04:12:47PM +0200, Alexander Graf wrote:
>> On 04/17/2012 05:28 PM, Andreas Färber wrote:
>>> Am 14.04.2012 04:07, schrieb Michael Roth:
>>>> 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<mdroth@linux.vnet.ibm.com>
>>> Tested-by: Andreas Färber<afaerber@suse.de>
>> Any conclusions? Today's tree is still broken for me.
> I didn't have anything in my qemu-ga queue so I submitted through
> qemu-trivial:
>
> http://comments.gmane.org/gmane.comp.emulators.qemu/146895
>
> Since it's a build fix it probably makes more sense to just send a Anthony a
> small PULL though so I'll send that out shortly.

Alrighty :). Just wanted to make sure it's not lost.


Alex

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index faf970d..087c3af 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -881,46 +881,50 @@  error:
 
 #else /* defined(__linux__) */
 
-GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
+void qmp_guest_suspend_disk(Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
-
-    return 0;
 }
 
-int64_t qmp_guest_fsfreeze_freeze(Error **err)
+void qmp_guest_suspend_ram(Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
-
-    return 0;
 }
 
-int64_t qmp_guest_fsfreeze_thaw(Error **err)
+void qmp_guest_suspend_hybrid(Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
-
-    return 0;
 }
 
-void qmp_guest_suspend_disk(Error **err)
+GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
 {
-    error_set(err, QERR_UNSUPPORTED);
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
 }
 
-void qmp_guest_suspend_ram(Error **err)
+#endif
+
+#if !defined(CONFIG_FSFREEZE)
+
+GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
+
+    return 0;
 }
 
-void qmp_guest_suspend_hybrid(Error **err)
+int64_t qmp_guest_fsfreeze_freeze(Error **err)
 {
     error_set(err, QERR_UNSUPPORTED);
+
+    return 0;
 }
 
-GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
+int64_t qmp_guest_fsfreeze_thaw(Error **err)
 {
-    error_set(errp, QERR_UNSUPPORTED);
-    return NULL;
+    error_set(err, QERR_UNSUPPORTED);
+
+    return 0;
 }
 
 #endif