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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

Hi,

On 06/21/2018 05:19 PM, Murilo Opsfelder Araujo wrote:
> 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.

The idea was to present the patches as incremental cleanups from
the existing code up to the point where the new API can be gracefully
added.

I think there's a point to be made about squashing all cleanups patches
in a single (or fewer) patches, but I believe it would be too annoying to
review that many changes at once - things started very coupled with
QMP functions hard-wiring pmutils binaries as parameters to the callers
(bios_support_mode and guest_suspend). If you compare that to the
result up to patch 4/6 you'll see that a lot was changed, and systemd
support was yet to be added.

Following the same incremental idea, patch 6/6 was created after I've
noticed that bios_support_mode was too sub-optimal after adding systemd
support. Yes, one can say that it was already sub-optimal before and it
should be removed right at the start, but I didn't bothered with this
change at that point due to other cleanups.

Anyway, if more people feels like the series structure should be changed
I can re-send the series again.

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

Didn't noticed it. If I end up respinning this series I'll remove it.

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

As I've mentioned in the commit message of patch 5/6, one step at a
time. If the function can be used in other places inside commands-posix.c
(IMO a fair assumption, but I didn't look it up carefully), then we could
first use this new function inside commands-posix.c and, if the function
turns out to be useful elsewhere, moved it to /util.

I didn't want to sound "pretentious" by adding a new function to /util
just because it is being used by the code I'm sending and because I
have a hunch that it will be useful to everyone else.



Thanks,


Daniel

>
> Cheers
> Murilo
>