Re: [PATCH 3/4] XenAgent: Add retry for system shutdown/restart

[Paul Durrant <xadimgnik@xxxxxxxxx>]

On 31/01/2024 08:12, Owen Smith wrote:
> In some situations, InitiateSystemShutdownEx can fail and not trigger the
> shutdown/restart correctly, leading to toolstack failing the shutdown/restart
> operation. Add a simple retry mechanism to attempt to trigger the
> shutdown/restart several times before giving up.
>
> Suggested-by: Martin Harvey <martin.harvey@xxxxxxxxxx>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>
> ---
>   src/xenagent/service.cpp        | 13 ++++-
>   src/xenagent/xenifacedevice.cpp | 86 +++++++++++++++++++++++++--------
>   src/xenagent/xenifacedevice.h   |  5 ++
>   3 files changed, 83 insertions(+), 21 deletions(-)
>
> diff --git a/src/xenagent/service.cpp b/src/xenagent/service.cpp
> index 535d761..ec32ecc 100644
> --- a/src/xenagent/service.cpp
> +++ b/src/xenagent/service.cpp
> @@ -202,8 +202,13 @@ bool CXenAgent::ServiceMainLoop()
>       HANDLE  events[] = { m_svc_stop,
>                            m_xeniface.m_evt_shutdown,
>                            m_xeniface.m_evt_suspend,
> -                         m_xeniface.m_evt_slate_mode };
> -    DWORD   wait = WaitForMultipleObjectsEx(4, events, FALSE, timeout, TRUE);
> +                         m_xeniface.m_evt_slate_mode,
> +                         m_xeniface.m_evt_shutdown_retry };
> +    DWORD   wait = WaitForMultipleObjectsEx(sizeof(events) / sizeof(events[0]),
ARRAYSIZE() or std::size() perhaps? I'll adjust. Rest of it looks fine.

Acked-by: Paul Durrant <paul@xxxxxxx>

> +                                            events,
> +                                            FALSE,
> +                                            timeout,
> +                                            TRUE);
>   
>       switch (wait) {
>       case WAIT_OBJECT_0:
> @@ -229,6 +234,10 @@ bool CXenAgent::ServiceMainLoop()
>   
>           return true; // continue loop
>       }
> +    case WAIT_OBJECT_0+4:
> +        m_xeniface.CheckShutdownRetry();
> +        return true; // continue loop
> +
>       case WAIT_TIMEOUT:
>           m_xeniface.CheckXenTime();
>           __fallthrough;
> diff --git a/src/xenagent/xenifacedevice.cpp b/src/xenagent/xenifacedevice.cpp
> index 9ed1aa7..223d6e9 100644
> --- a/src/xenagent/xenifacedevice.cpp
> +++ b/src/xenagent/xenifacedevice.cpp
> @@ -40,6 +40,9 @@
>   #include "xeniface_ioctls.h"
>   #include "messages.h"
>   
> +#define DEFAULT_SHUTDOWN_RETRIES        5
> +#define DEFAULT_SHUTDOWN_RETRY_TIME     60
> +
>   CXenIfaceDevice::CXenIfaceDevice(const wchar_t* path) : CDevice(path)
>   {}
>   
> @@ -178,16 +181,20 @@ CXenIfaceDeviceList::CXenIfaceDeviceList(CXenAgent* agent) : CDeviceList(GUID_IN
>       m_agent(agent),
>       m_ctxt_suspend(NULL),
>       m_ctxt_shutdown(NULL),
> -    m_ctxt_slate_mode(NULL)
> +    m_ctxt_slate_mode(NULL),
> +    m_shutdown_reboot(false),
> +    m_shutdown_retry(0)
>   {
>       m_evt_shutdown = CreateEvent(NULL, TRUE, FALSE, NULL);
>       m_evt_suspend = CreateEvent(NULL, TRUE, FALSE, NULL);
>       m_evt_slate_mode = CreateEvent(NULL, TRUE, FALSE, NULL);
> +    m_evt_shutdown_retry = CreateWaitableTimer(NULL, FALSE, NULL);
>       m_count = 0;
>   }
>   
>   /*virtual*/ CXenIfaceDeviceList::~CXenIfaceDeviceList()
>   {
> +    CloseHandle(m_evt_shutdown_retry);
>       CloseHandle(m_evt_slate_mode);
>       CloseHandle(m_evt_suspend);
>       CloseHandle(m_evt_shutdown);
> @@ -301,29 +308,21 @@ bool CXenIfaceDeviceList::CheckShutdown()
>           m_agent->EventLog(EVENT_XENUSER_POWEROFF);
>           LogIfRebootPending();
>   
> -        AcquireShutdownPrivilege();
> -#pragma warning(suppress:28159) /* Consider using a design alternative... 
> Rearchitect to avoid Reboot */
> -        if (!InitiateSystemShutdownEx(NULL, NULL, 0, TRUE, FALSE,
> -                                      SHTDN_REASON_MAJOR_OTHER |
> -                                      SHTDN_REASON_MINOR_ENVIRONMENT |
> -                                      SHTDN_REASON_FLAG_PLANNED)) {
> -            CXenAgent::Log("InitiateSystemShutdownEx failed %08x\n", 
> GetLastError());
> -        }
> -        return true;
> +        m_shutdown_retry = DEFAULT_SHUTDOWN_RETRIES;
> +        m_shutdown_reboot = false;
> +
> +        RequestSystemShutdown();
> +        return false; // keep service running, to retry shutdown if needed.
>       } else if (type == "reboot") {
>           device->StoreWrite("control/shutdown", "");
>           m_agent->EventLog(EVENT_XENUSER_REBOOT);
>           LogIfRebootPending();
>   
> -        AcquireShutdownPrivilege();
> -#pragma warning(suppress:28159) /* Consider using a design alternative... 
> Rearchitect to avoid Reboot */
> -        if (!InitiateSystemShutdownEx(NULL, NULL, 0, TRUE, TRUE,
> -                                      SHTDN_REASON_MAJOR_OTHER |
> -                                      SHTDN_REASON_MINOR_ENVIRONMENT |
> -                                      SHTDN_REASON_FLAG_PLANNED)) {
> -            CXenAgent::Log("InitiateSystemShutdownEx failed %08x\n", 
> GetLastError());
> -        }
> -        return true;
> +        m_shutdown_retry = DEFAULT_SHUTDOWN_RETRIES;
> +        m_shutdown_reboot = true;
> +
> +        RequestSystemShutdown();
> +        return false; // keep service running, to retry shutdown if needed.
>       } else if (type == "s4") {
>           device->StoreWrite("control/shutdown", "");
>           m_agent->EventLog(EVENT_XENUSER_S4);
> @@ -347,6 +346,12 @@ bool CXenIfaceDeviceList::CheckShutdown()
>       return false;
>   }
>   
> +void CXenIfaceDeviceList::CheckShutdownRetry()
> +{
> +    CXenAgent::Log("Previous shutdown attempt failed, retrying...\n");
> +    RequestSystemShutdown();
> +}
> +
>   void CXenIfaceDeviceList::CheckXenTime()
>   {
>       CCritSec crit(&m_crit);
> @@ -476,6 +481,49 @@ void 
> CXenIfaceDeviceList::AdvertiseFeatures(CXenIfaceDevice* device, bool add)
>       device->StoreWrite("control/feature-laptop-slate-mode", value);
>   }
>   
> +#define TIME_US(_us)            ((_us) * 10ll)
> +#define TIME_MS(_ms)            (TIME_US((_ms) * 1000ll))
> +#define TIME_S(_s)              (TIME_MS((_s) * 1000ll))
> +#define TIME_RELATIVE(_t)       (-(_t))
> +
> +void CXenIfaceDeviceList::RequestSystemShutdown()
> +{
> +    LARGE_INTEGER dueTime;
> +
> +    AcquireShutdownPrivilege();
> +
> +#pragma warning(suppress:28159) /* Consider using a design alternative... 
> Rearchitect to avoid Reboot */
> +    if (!InitiateSystemShutdownEx(NULL,
> +                                  NULL,
> +                                  0,
> +                                  TRUE,
> +                                  (m_shutdown_reboot ? TRUE : FALSE),
> +                                  SHTDN_REASON_MAJOR_OTHER |
> +                                  SHTDN_REASON_MINOR_ENVIRONMENT |
> +                                  SHTDN_REASON_FLAG_PLANNED)) {
> +        CXenAgent::Log("InitiateSystemShutdownEx failed %08x\n", 
> GetLastError());
> +    }
> +
> +    if (m_shutdown_retry == 0) {
> +        CXenAgent::Log("System not shutdown after several tries, giving 
> up.\n");
> +        return;
> +    }
> +
> +    m_shutdown_retry--;
> +
> +    // Negative (i.e. relative) number of minutes to wait in 100ns intervals.
> +    dueTime.QuadPart = TIME_RELATIVE(TIME_S(DEFAULT_SHUTDOWN_RETRY_TIME));
> +
> +    if (!SetWaitableTimer(m_evt_shutdown_retry,
> +                          &dueTime,
> +                          0,
> +                          NULL,
> +                          NULL,
> +                          FALSE)) {
> +        CXenAgent::Log("SetWaitableTimer failed %08x\n", GetLastError());
> +    }
> +}
> +
>   void CXenIfaceDeviceList::AcquireShutdownPrivilege()
>   {
>       HANDLE          token;
> diff --git a/src/xenagent/xenifacedevice.h b/src/xenagent/xenifacedevice.h
> index 29ac100..5a1aecf 100644
> --- a/src/xenagent/xenifacedevice.h
> +++ b/src/xenagent/xenifacedevice.h
> @@ -81,9 +81,11 @@ public:
>       HANDLE  m_evt_shutdown;
>       HANDLE  m_evt_suspend;
>       HANDLE  m_evt_slate_mode;
> +    HANDLE  m_evt_shutdown_retry;
>   
>       void Log(const char* message);
>       bool CheckShutdown();
> +    void CheckShutdownRetry();
>       void CheckXenTime();
>       void CheckSuspend();
>       bool CheckSlateMode(std::string& mode);
> @@ -95,6 +97,7 @@ private:
>       void StartSlateModeWatch(CXenIfaceDevice* device);
>       void StopSlateModeWatch(CXenIfaceDevice* device);
>       void AdvertiseFeatures(CXenIfaceDevice* device, bool add);
> +    void RequestSystemShutdown();
>       void AcquireShutdownPrivilege();
>       void SetXenTime(CXenIfaceDevice* device);
>   
> @@ -104,6 +107,8 @@ private:
>       void*       m_ctxt_suspend;
>       void*       m_ctxt_shutdown;
>       void*       m_ctxt_slate_mode;
> +    bool        m_shutdown_reboot;
> +    DWORD       m_shutdown_retry;
>   };
>   
>   #endif