Re: [Qemu-devel] [PATCH v2 0/6] QGA: systemd hibernate/suspend/hybrid-sleep

[Murilo Opsfelder Araujo]

On Thu, Jun 21, 2018 at 07:21:47AM -0300, Daniel Henrique Barboza wrote:
> changes in v2 from Marc-Andre Lureau review:
> - use error_free() accordingly
> - use g_spawn_sync() instead of fork() in run_process_child()
> - previous version link:
> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg05499.html
> 
> 
> This series adds systemd suspend support for QGA. Some newer
> guests don't have pmutils anymore, leaving us with just the
> Linux state file mechanism to suspend the guest OS, which does
> not support hybrid-sleep. With this implementation, QGA is
> now able to hybrid suspend newer guests again.
> 
> Most of the patches are cleanups in the existing suspend code,
> aiming at both simplifying it and making it easier to extend
> it with systemd.
> 
> 
> Daniel Henrique Barboza (6):
>   qga: refactoring qmp_guest_suspend_* functions
>   qga: bios_supports_mode: decoupling pm-utils and sys logic
>   qga: guest_suspend: decoupling pm-utils and sys logic
>   qga: removing switch statements, adding run_process_child
>   qga: systemd hibernate/suspend/hybrid-sleep support
>   qga: removing bios_supports_mode

Hi, Daniel.

Your patches look good, just some minor comments about how they're
organized and coding style, if you allow me.

I'd suggest to introduce the new API, functions, and typedef at first
(preferably one thing per patch to easier the review) without wiring
them up.

After the new stuff is ready to use, I'd wire them up (one per patch)
and update/remove old API they're replacing.

Introducing the new API and wiring it up later makes it easier to
review.  For example, bios_supports_mode() is changed by patch 1/6, then
it's moved around by patch 3/6, and finally removed by patch 6/6.

Do we need an empty line after opening a switch statement?  I'd drop it,
as seen in other parts of the code.

Does run_process_child() fit better under util/, or another place where
it can be shared throughout the code, if that's the case?

Cheers
Murilo