init_sdr assigned self._sdr before initialize() ran, so a failure left the
half built object in the cache. The next call saw a non-None _sdr and handed
back that partial repository rather than trying again.
The visible symptom is a first call raising and the second appearing to
succeed. The real cost is on a bmc where the read fails once: the client
keeps the incomplete sdr for the life of the session and every later sensor
lookup answers from it without complaint.
Two places where the code that exists to explain a failure fails instead, and
the caller is shown the second failure rather than the first.
_do_web_request builds its message from the response body when that body is
not the JSON error document the spec asks for. An html 404 page is exactly
that, the body is bytes, and str + bytes raised TypeError. The status and the
body never reached anyone.
LenovoFirmwareConfig raised a bare Exception when python-lxml and
python-eficompressor are absent. Confluent has no handler for one, so a
missing dependency showed as "Unexpected Error" and hid a message that
already said what to install.
Neither changes what fails, only what the caller is told.
A bmc behind a forward answers Activate Payload with the port it listens on
itself, and the advertised-port check refused that, so a console failed where
command traffic worked. The advertised port is never sent to, so the check now
applies only on the default port. A bmc on another port advertising a third one
is no longer refused outright, which nothing here could have served anyway.
A device holds its sensor records per lun and scopes the reservation the
same way, so a token taken on lun 0 can be refused for lun 1 with 0xc5.
The retry then took the same wrong token again without end.
pyghmi asks for this parameter with xraw_command and catches the completion
code for a bmc that does not have it, and folding aiohmi in renamed that call
to oldraw_command rather than raw_command, so the handler could no longer fire.
Answering the code out of the returned dictionary repaired the crash but kept
the call on the older contract, which is now the only one left in the tree.
Catch it again instead: raw_command puts the completion code on the exception
as ipmicode, and nothing here reads the payload of a reply that carries a code,
which is the one thing catching gives up.
No behaviour change, checked against the previous version over the same fake
session for a good reply, an empty one, 0x80 and 0xC9 with and without a stray
payload, four other completion codes, a timeout, a lost session and a reply
with no data at all: same return value, same exception type, text and ipmicode,
same bytes on the wire.
_read_device_sdr_lun negotiates the read size down when the bmc answers 0xCA,
but the size > 5 guard leaves a size of 5 alone, so a bmc that will not serve
5 bytes at once was asked the same question for ever. Give up once the
request cannot get any smaller, and once a header read would go under the 5
bytes the record length sits in, by falling through to the raise already
there.
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, so the very first request repeated unchanged. Take one where
the code is handled.
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.
get_user_name documents that it answers None when reading a slot fails, but
raw_command raises before the check that would return it, so that branch has
never run and one refused slot aborted the whole user list. Answer None where
the docstring says to, and let get_users drop the blanket except it grew to
work around it.
Only a completion code counts as the bmc answering about the slot. A timeout
carries the fabricated 0xffff from the session layer and a lost session carries
no code at all, and swallowing either of those reports a list truncated at the
point of failure as a complete one.
The sdr carries unit fields for every sensor record, and they were read into
the reading whether or not the sensor has a number for them to describe. A
discrete sensor reports which of its states are asserted and no value at all,
so a watchdog came back with units of "% ", and a discrete sensor on a full
record picks up a base unit the same way, reporting degrees celsius for a
sensor that never has a temperature. A caller that shows the unit alongside
whatever value it was handed then prints a unit with nothing to apply it to,
which is what the client was taught to skip in 8c8c32cc. The client is not
the only consumer of a reading, so answer the question in the library.
The record itself says which sensors those are, and the reading path already
works it out to decide whether to decode a number: a sensor has one only if
the numeric format says how to read it, or the format is unsigned and the
record either supports thresholds or is of reading type 1. Ask that once,
where the units are assembled, and give the sensors that fail it no unit, so
the unit and the value cannot come to different conclusions about whether the
sensor has a reading at all.
A threshold or numeric sensor reports exactly what it did before, including
the ones whose unit is a percentage, a combination of two units, or nothing
because the record names no unit. On the Lenovo XCC this was checked against
nothing changes: its 305 readable sensors decode identically, discrete ones
included, because they are all compact records naming no unit in the first
place. The sensors that change are the ones the finding came from, which
name a unit on a record that has no number to put it on.
The top bit of the major revision byte of Get Device ID says the device is
still initialising or taking a firmware update. It was read as part of
the revision, so immediately after a bmc reset nodefirmware reported "BMC
Version: 131.11" for what is 3.11, and settled down only once the bit
cleared.
Mask it as sdr.py already does for the same byte, so the two agree about
the same field.
The loop decoded the first three bytes of a name and never consumed them,
so any sensor or fru name a bmc encodes as 6 bit packed ascii spins at
full speed, appending the same four characters until the process runs out
of memory. Measured at about 10 MB a second, so a bmc using an encoding
the spec gives its own worked example of costs a pinned core and, before
long, the daemon.
Consume each group, and decode a trailing group of one or two bytes rather
than dropping it, since those carry a character each and the name would
otherwise come back short.
The arithmetic was already right, it was only never reached a second time.
Verified by encoding names per the packing and reading them back: exact for
every length except those leaving three characters in a three byte group,
where the byte count cannot say whether three or four were meant and a
trailing space is unavoidable.
A bmc may keep its sensor data records on the sensor device instead of in
a repository, and this one does, so it had no sensors, no health and only
a partial inventory over ipmi.
The records themselves are identical, version 0x51 and the same types, so
everything that decodes them is reused as is. Only the fetching differs:
a command of its own, a reservation of its own, and records held per lun
rather than in one place. The luns to ask, and a change indicator to
cache on, come from Get Device SDR Info.
The fetch is written out rather than shared with the repository one. The
loops are alike, but nothing available here has a repository to test
against, and the price of factoring them together is that a mistake would
land on every bmc that works today rather than only on those that do not
work at all.
Records are cached in memory and on disk exactly as repository records
are, keyed on the change indicator, and a device that offers no such
indicator is read afresh each time rather than cached wrongly.
Names are stripped of the nulls that pad a fixed width field. This bmc
pads every name out to sixteen bytes, and a name carrying them cannot be
matched by a caller asking for a sensor by name.
On the bmc this was written for: 163 sensors and 9 frus, against the 163
the device reports it has. The 54 temperatures and 36 fans it now reads
match what the same machine reports over redfish, to within the precision
each side gives.
The oem lookup answers whether it found a handler for the vendor, and that
answer was being stored as whether the lookup had been done at all. On
anything the map does not name, which is every bmc that is not a Lenovo,
the flag stayed false and each oem_init issued another Get Device ID and
built another handler.
Almost everything goes through oem_init, so this is a round trip added to
almost every operation. Where those calls are close together it is far
worse than that: reading the sensor data records asks for the event
constants once per record, so a run of 172 records fired 176 Get Device ID
commands back to back, which was enough to make the bmc stop answering and
the read fail with a timeout. The same sequence now takes 3 commands.
Settling for the generic handler is an answer. The device id cannot
change within a session, so asking again buys nothing, and the handler it
throws away each time is the one holding the sensor names it had cached.
A bmc that keeps its sensor data records on the sensor device rather than
in a repository was answered with a bare NotImplementedError. With no
text of its own it reached the user as "Unexpected Error:
NotImplementedError" and was logged with a traceback, for a capability the
library had simply never implemented rather than anything having gone
wrong.
Say so instead, in all three places that give up: the two branches for a
bmc without an sdr repository, and the version check that only understands
records of version 0x51, which now names the version it was given.
This makes the inventory usable on such a bmc as a side effect. The
system fru is gathered before the records are, and an unsupported
operation is tolerated where an unexpected error was not, so nodeinventory
answers with the board, chassis and product data instead of one line of
error.
Reading the alert destinations of a bmc that has none reported "Unknown
code 0x80 encountered", which is the fallback text for a completion code
the library has no name for. 0x80 on this parameter is not a failure, it
is the platform saying it does not have alert destinations, and the
redfish side of the same resource has said so in words for a while.
The lan parameter fetch already knew how to tell those apart, so build the
alert reads on it rather than on a raw command that raises on any non-zero
code, and raise UnsupportedFunctionality with something to read. Both the
count and an individual destination are covered, so a platform that offers
one and not the other says the same thing instead of failing differently.
Splitting the completion code handling out of the parameter fetch is what
makes that reuse possible; the interpretation of the payload, and every
answer it gives, is unchanged.
The oem hook for the destination count was passing its byte through ord(),
which raises TypeError on the bytearray it is given. No handler in tree
implements the hook, so it had never been called; hand it the integer.
A bmc that does not implement a lan configuration parameter says so in the
completion code and answers with no data at all. The helper reached
straight into the payload, so such a parameter raised IndexError, and with
it went the whole of nodeconfig over ipmi: the bmc group, the plain, the
detailed, the extended and the advanced reads all ended in "bytearray
index out of range". It also took the attribute enumeration with it, so
the client then rejected names it should have accepted.
The guard that was there caught an exception carrying the completion code,
but oldraw_command reports the code in its response rather than raising on
it, so nothing was ever caught. Read the code from the response instead:
parameter not supported and parameter out of range mean the platform does
not have it, and anything else is a real failure that should say what it
was rather than be mistaken for absence.
The address configuration method was looked up in a table with no regard
for whether it had been read at all, so a bmc that does not report it
would have traded the IndexError for a KeyError. Answer None when it is
absent, as the address above it already does, and name the value when it
is present but unfamiliar.
nodefirmware <node> disks reported the bmc version, and so did adapters and
misc. The generic handler takes a category and ignores it, and the ipmi plugin
does not filter either, so every category answered with the one entry ipmi can
report.
Apply the rule R13 established for a redfish inventory that does not categorise
itself: the bmc's own firmware is system firmware, so it answers for core and
for nothing else. Filtering in the handler that produces the entry leaves an oem
handler that does categorise its own firmware free to answer for more.
There is no ipmi command for a bmc hostname, so get_hostname falls back to the
DCMI management controller identifier. A bmc that does not implement the DCMI
group at all rejects that with "invalid command", which was handed to the caller
as if the request had been bad: nodeconfig <node> bmc read seven fields
correctly and then reported "Error: Invalid command", and the api answered 500
Unexpected error.
Route every DCMI request through one helper that turns "invalid command" and
"command disabled or unavailable" into UnsupportedFunctionality, so the
identifier, the asset tag and the hostname all report a platform that cannot do
this rather than a fault. Where the caller asked about a hostname, say that
rather than naming DCMI.
The whole group read still ends in one error line, because an operation a
platform cannot perform and one that failed are the same message to the client.
That is worth separating, but not here.
A bare UnsupportedFunctionality() left the user with an error containing no
text at all, or with no output and a success exit code, so asking a platform
for something it does not implement looked like nothing had happened.
Name what is unsupported at each raise, treat it as its own case in the
plugins so it reads as a limitation rather than an unexpected error and does
not log a traceback, and fall back to naming the exception when an exception
still arrives with nothing to say. The generic redfish
get_extended_bmc_configuration was also declared without async while the
caller awaits it.
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.
handle_users iterated get_users with async for while the same call is awaited
a few lines below, so listing the users collection, and creating a user,
raised a TypeError about a coroutine having no __aiter__. list_inventory in
the redfish plugin had the mirror of it, awaiting an async generator.
get_extended_bmc_configuration is called with hideadvanced but the ipmi chain
never accepted it, so the extra and extra_advanced resources raised a
TypeError; thread the argument through instead.
A user slot the bmc refuses to describe no longer takes the whole user list
with it: one MegaRAC slot answered Invalid data field for good after an
account was deleted, which broke every user operation.
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.
Markus Hilger
Jarrod Johnson
Jarrod Johnson