Completion codes 201 and 202 mean the chunk asked for was too big, and the
check for them sat after a call that raises, so a bmc that cannot serve 224
bytes at once failed the fru read rather than being asked for less.
The retry could not terminate either: chunksize // 2 + 2 is 4 for a chunksize
of 4, so the chunksize == 3 guard was unreachable and a bmc that kept refusing
would have been asked for 4 bytes for ever.
Completion code 203 on a sensor reading means the sensor is not present, which
is expected of an optional device, but the check for it sat after a call that
raises first, so one absent sensor ended the whole sensor sweep.
_supports_standard_ipv6 read rsp['code'] the same way, so it could only ever
answer True; a platform without the standard parameters raised instead. A
completion code is that platform's answer, while a timeout or a lost session is
not and must not be cached as one.
raw_command's docstring still described itself as the other call it was renamed
from, which is how these checks came to be written against the wrong contract.
raw_command raises on any nonzero completion code, so the 0xCA and 0xC5 checks
in get_sdr could not run: a bmc that will not return a whole record in one go,
or whose reservation went stale, failed the sensor load outright instead of
being retried. Read the code from the exception, which carries it.
The back off also had a fixed point, size // 2 + 2 being 3 for a size of 3, so
a bmc that kept refusing would have been asked the same question for ever.
Give up when the request cannot get any smaller. A header read cannot go
under 5 bytes either, since that is where the record length sits, so give up
there rather than parse a reply too short to index.
The stale reservation retry could not end on its own either: it cleared the id
and left taking a new one to the top of the loop, which only reserves for a
partial read. Take one where the code is handled.
Closing a console gives up its claim on the session, and that talks to the
bmc, so let it fail the same way the console's own deactivate is already
allowed to.
kg was left as the caller passed it in both the register key and the reuse
check, so the mismatch fixed for the name and password still applied to it.
The count for a new socket was taken before binding it and before the io
task was known to be up. Take it last, once nothing is left that can still
fail.
Telling a keepalive that the session is gone can end up back in logout,
because reporting it is how a console gives up its claim. The inner pass
finished by clearing the register of keepalives while the outer was still
walking it, so the next entry was looked up on None. Two entries is what an
XCC has, the console's and the oem handler's.
Take the callbacks and give up the register before notifying anyone.
The check for sharing a live session compared the credentials a session
keeps encoded against the strings every caller passes, so it never matched
and each caller built another session beside the one it could not see.
Normalise both sides. Three commands to one bmc went from three sessions on
three sockets to one, and from five sessions open on the bmc to three.
Not from the port to asyncio: upstream compares the same two things the same
way.
One session is now routinely handed to a console and a command at once, and
logout closed it for both, leaving whoever was left holding one that
answered as though it had been lost. Count the holders and give up a claim
instead, unless the session is no longer usable, which logout is told by
sessionok.
A console had no way to give a claim back: close deactivated its sol payload
and left the session alone, which was right when closing meant closing it
for everybody and is a leak now. Both of its exits release it.
__init__ opened by checking for an initialized attribute, meaning it had
been handed a session someone else was establishing. That attribute is only
ever set further down in the same method, so the check cannot be true, and
the port lost the return that made it work upstream. Waiting for someone
else's login is done in __new__ now, so this is dead code claiming to
protect something.
logout decremented the count of sessions on a socket twice over, and
_mark_broken again for the case logout had not, so it went negative and kept
falling. _assignsocket picks the least used socket and refuses one at
MAX_BMCS_PER_SOCKET, and both of those read that number.
initting_sessions exists so a caller can share a session already on its way,
but the entry was added only once the login had finished, leaving the login
itself uncovered. Two callers asking at once each built a session, both on
the socket the other had not claimed yet, and replies route by bmc address
and local port, so only the last to transmit was ever answered. That is the
console session failing about one attempt in three.
Register before the login and remove the entry in a finally. The two old
removals keyed on the encoded credentials while the register is keyed on the
caller's strings, so they never matched and an entry outlived its session.
A session handed over mid login is now waited for rather than returned as
one that answers as though it had been lost.
A session that failed raised with no message at all, so a failed console
read "IpmiException: None". Record the reason wherever a session is marked
broken.
Four calls reached a base method with an argument list it does not accept,
so they raised TypeError about the argument count.
Three of them would have failed either way, since the base only raises
UnsupportedFunctionality. What changes there is that the failure becomes
the intended, catchable one rather than an argument count error the caller
cannot interpret. get_diagnostic_data grew an autosuffix argument
everywhere except the ipmi generic handler, which is the handler used for
unrecognized hardware. The redfish generic handler already had it. The two
storage super() calls dropped the cfgspec they were given.
The fourth is a real fallback rather than a message: the XCC user_delete
dropped the fishclient it receives from redfish/command.py, so deleting a
uid the XCC does not list raised TypeError instead of attempting the
generic Redfish delete.
Each is the only thing keeping its rule group from being selectable whole.
userutil.py imported ctypes with a star; the names it uses are POINTER,
byref, c_char_p, c_int, c_int32, c_uint and cdll. The oem lookup loop had an
else with no break, so the else always ran. The alert parameter table wrapped
int in a lambda that only forwards to it. And the watchdog interval passed 0
where os.environ.get documents a string, which worked because int(0) is 0.
get_sensor_names and get_sensor_descriptions reach get_psu_count for any
sensor whose table entry carries elementsfun, and get_psu_count is a
coroutine. As plain generators they could not await it, so range() was handed
the coroutine object and enumeration died with "'coroutine' object cannot be
interpreted as an integer".
Every DW612S has such entries, so nodesensors returned nothing for the
enclosure. get_sensor_descriptions was doubly broken: the Lenovo handler
already iterated it with async for, which a plain generator cannot satisfy.
Verified against a DW612S SMM (FPC variant 38). Before, descriptions raised at
the async for and readings raised partway through enumeration; after, both
return all 34 sensors, 19 of which are the PSU entries that never enumerated.
The console awaits its output handler, but both sample BMCs supplied a plain
function, and both dropped the send_data coroutine. virshbmc additionally
receives its stream callback on a libvirt thread, so the send goes through
run_coroutine_threadsafe against the loop captured at activation, called
asyncloop because the class already has a loop method it uses as a thread
target.
ServerConsole asked the session layer to retry, which a ServerSession cannot
do: it never runs Session.__init__, so it has no timeout, and its _timedout
does nothing. IpmiServer.logout was synchronous and an argument short while
_cleanup awaits logout(False). The boot options handler answered, then read an
unbound name and answered again with 0xff.
bmc.py, serversession.py, fakebmc.py and virshbmc.py were byte identical to
upstream pyghmi: the async port went through the session layer beneath them
and left the server side alone. So every response created a coroutine and
dropped it, and the overrides the now async parent awaits returned None.
Running fakebmc bound no socket, spun a core, and answered nothing.
Everything that sends a response is a coroutine now, and so is the dispatch
that reaches it; the hooks a subclass implements stay ordinary functions, so
an out of tree Bmc is unaffected unless it overrides the payload handlers.
Two things had to leave their constructors, both being coroutines: assigning
the server socket, into bind(), which is why listen() is no longer a
classmethod, and answering the open session request, into
send_open_session_response.
Verified against upstream with ipmitool over nine commands, with identical
output. SOL is not covered: fakebmc reports the payload disabled on both.
Housekeeper dates from when this work was a blocking select loop. Under
asyncio the event loop is already that place, and a thread around it takes a
captured loop reference, a scheduling step before the thread starts, and a
daemon flag, only to sit waiting on a coroutine that never returns with no way
to stop it. A task runs on the right loop by construction and cancels. The
loop is looked up before the coroutine is built, so calling this without one
raises rather than stranding it.
Nothing in this tree used the class. Out of tree callers need
start_housekeeping() instead, and gain the ability to stop it.
Command.eventloop called wait_for_rsp with no timeout. With nothing waiting or
being kept alive there is no deadline to derive one from, so it returns
without suspending and the loop runs flat out, measured at over 100000
iterations in two tenths of a second.
MAX_IDLE gives it something to wait on, as a ceiling rather than a fixed
delay: real deadlines still shorten it and an arriving packet still ends it
early.
Console.main_loop drives Session.wait_for_rsp, which is a coroutine, so it
spun without ever waiting for a packet. It is a coroutine now, and pyghmicons
runs its main under asyncio.run. pyghmiutil had the same shape around
Command.eventloop.
sockapi sent its collective refusal without awaiting tlvdata.send. redfish
handle_sensors returned a coroutine from most branches and None from the short
ones, so the caller's await raised TypeError; it is a coroutine throughout
now. console send_payload waited for a response without awaiting the wait.
Two suppressions are pyrefly limitations rather than bugs: Session defines an
async __new__, and the keepalive registry holds coroutine functions in an
untyped dict.
Three names were defined twice in the same scope, so the first definition
was unreachable:
- lenovo OEM handler: two set_user_access methods, the second silently
replacing the first. That made the SMM privilege update dead code. The
conditions are mutually exclusive (is_fpc returns None once has_xcc is
true), so merge both into the surviving method.
- redfish plugin handle_cert_authorities and prepfish
disable_host_interface: byte identical copies, drop the redundant one.
grab_response_with_status hands back bytes, so every failure path put a
b'<status>error</status>' repr in front of the operator rather than what
the SMM said. Decode at the raise, replacing rather than failing on a
body that is not valid utf8. The bodies still reach fromstring() as
bytes, which is what lxml wants when the xml carries an encoding
declaration.
The poll loop spends its retry budget on a poll that goes unanswered but
aborted the update on the first non-200, even though an SMM restarting
its web service part way through the apply keeps answering, with
whatever its httpd has to say, before it stops answering at all. Give a
bad status the same budget as a dead connection.
Every other /data call checks the status, this one discarded the
response, so an SMM that refused the user record was reported to the
caller as a successful privilege change.
Staleness is judged by age alone, so a session the SMM ended on its own
reached the operator as a raw error body instead of being retried.
Route the /data calls through a helper that logs back in and retries
once on a 401, which is how the SMM answers once a session is gone.
A hostname or domain write does not end the session, measured on a
DW612S at firmware 1.18, so this covers what the chassis drops by
itself, not a self-inflicted loss.
The counter is there to ride out a few unanswered progress polls, but
nothing ever cleared it, so three failures spread across a long apply
exhausted it and aborted an update that was still making progress.
A firmware update posts on one session for the minutes its apply loop
runs, and an FFDC collection downloads on the session it acquired, but
wc() judges a session by its age alone, so a settings call arriving
thirty seconds in logged that session out from underneath them. Flag
the long operations the way the IMM and XCC handlers already do and
leave their session in place.
Every getter and setter logged out on the way out, which nulled the
cached client and made the session cache inert on exactly the paths it
was meant to serve: a single nodeconfig walk of ntp costs two full
logins for the read and one per server for the write, each of them a
fresh TLS handshake plus, on firmware that omits st2, two extra page
fetches to scrape the tokens.
Leave the session in place and let wc() dispose of it once it expires.
This also stops one coroutine's logout from invalidating the session
another coroutine just fetched and is about to post with.
The retry counter is there to ride out a few unanswered polls, but
exhausting it broke out of the loop with complete still unset and fell
through to the 'complete' return, so an SMM that stopped answering
part way through an apply was reported to the operator as updated.
Raise instead; a genuine finish still leaves the loop on the progress
reaching 100.
Now that the expiry comparison actually caches a client, the session it
holds is shared, so tearing it down and replacing it needs the same care
the IMM handler already takes:
Dispose of an expired session with a logout instead of dropping the
reference, otherwise every refresh leaves an authenticated session
behind on an SMM that only has a handful of slots. That logout has to
tolerate a session the SMM has already reaped, hence the except.
Guard the login itself, so two coroutines arriving at an empty or
expired cache do not both log in and orphan one of the two sessions.
Stamp the vintage once the login round trips are done rather than
before, so a slow SMM cannot hand back a client that is already expired.
The old WebConnection.request() added a
'Content-Type: application/x-www-form-urlencoded' header to any POST
carrying a body, but grab_response_with_status() only sets a content
type for dict payloads, so the SMM login and every /data form POST now
go out as text/plain. This is not a fix for an observed failure: an SMM
running FPC variant 38 was measured accepting a text/plain login exactly
as readily as a urlencoded one. It restores the header the synchronous
code always sent and that the TSM and IMM handlers still set explicitly,
rather than relying on every SMM firmware level being equally lax about
what it will parse.
Also stop hard failing on responses the synchronous code discarded on
purpose. 'set=securityrollback:1' is only understood by newer SMM2
firmware. And /data/logout answers 401 once the session is gone, as
measured on that same SMM, so raising on a non-200 there turns a
completed hostname, domain or NTP operation into a spurious error.
The SMM hostname, domain, and NTP helpers looked synchronous even though their web transport is asynchronous. Removing awaits in the Lenovo OEM handler therefore returned unresolved coroutine work instead of completed settings results.
Convert the SMM settings and logout helpers to the asynchronous web interface, validate HTTP status responses, and await each operation from the OEM handler so callers only observe completed results.
The NextScale SMM path still used the removed http.client-style interface against the asynchronous WebConnection implementation. Login, configuration, diagnostic, and firmware operations consequently called unavailable methods or left request coroutines unresolved.
Make web-client creation asynchronous, migrate the affected requests to grab_response_with_status(), and await the cached client accessor. Correct the cache expiry comparison so fresh authenticated clients are reused and stale clients are renewed.
Apply ruff's safe autofixes.
The changes are mechanical and behaviour-preserving. Issues fixed:
- F401: remove unused imports.
- F841: drop unused local variables and assignments, including discarded
await/return values, unused "except ... as e" bindings, and unused
"with ... as name" targets.
- F541: remove the f prefix from f-strings that contain no placeholders.
- E711: compare against None with "is"/"is not" instead of "=="/"!=".
- E712: test truthiness directly instead of comparing to True.
- E713: use "x not in y" instead of "not x in y".
- E714: use "is not" instead of "not ... is".
- E731: convert lambdas bound to a name into def statements.
- W291/W293: trim trailing whitespace on touched lines.
The IPMI 1.5 session-challenge callback returned coroutine objects from error reporting and session activation instead of expressing an asynchronous callback contract directly. That made completion depend on the dispatch path noticing and awaiting the returned object.
Make the callback asynchronous and explicitly await both onlogon() and _activate_session(), ensuring failure notification and activation finish before callback dispatch continues.
Await the channel access raw command, the TSMA remote media settings
requests, and the XCC3 volume creation responses. These calls returned
or unpacked coroutine objects, breaking set_channel_access, TSMA
virtual media attach, and RAID volume creation at runtime.
Await OEM sensor, NTP, retry, and firmware-update operations that otherwise returned or discarded coroutine objects. Return the initialized energy manager for FAPM systems and update stale utility entry points to use asynchronous Command factories.
A type 0x02 record whose body cannot be decoded (e.g. a bogus EvM
revision) would raise and abort retrieval of the entire event log.
ipmitool and freeipmi print such entries with whatever fields they can
extract rather than failing; degrade to the same raw passthrough used
for undecodable reserved types instead of raising.
The Linux kernel ipmi panic logger stores panic strings in SEL records
of type 0xf0, with a chunk sequence number in byte 4 and up to 11
characters of the message in bytes 5-15. ipmitool and freeipmi both
recognize this convention; do the same rather than presenting such
records as opaque non-timestamped OEM data.
Some BMCs (e.g. AMI) log events using spec-reserved record types like
0x04 with a standard system event record layout. Previously only type
0x02 was decoded, leaving such entries with no usable data and tripping
the generic OEM handler. Follow ipmitool and treat all types below 0xc0
as standard format. If the body of a reserved type turns out not to
follow the standard layout, fall back to passing it through raw instead
of aborting the whole log retrieval.
Markus Hilger
Jarrod Johnson