| Message ID | 1334369256-32663-1-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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
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__) */
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__) */ >
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
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 > >
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
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
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(-)