On 04/06/2024 18.27, Jared Rossi wrote:
Hi Thomas,
Firstly, thanks for the reviews and I agree with your suggestions about
function names, info messages, simplifications, etc... I will make those
changes.
As for the DIAG308_FLAGS_LP_VALID flag...
[snip...]
@@ -438,40 +473,20 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
break;
}
- if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
- ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
+ /* If the device loadparm is empty use the global machine
loadparm */
+ if (memcmp(lp, "\0\0\0\0\0\0\0\0", 8) == 0) {
+ lp = S390_CCW_MACHINE(qdev_get_machine())->loadparm;
}
+ s390_ipl_set_loadparm((char *)lp, ipl->iplb.loadparm);
+ ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
That means DIAG308_FLAGS_LP_VALID is now always set, even if no loadparm
has been specified? Shouldn't it be omitted if the user did not specify
the loadparm property?
No, I don't think it should be omitted if the loadparm hasn't been specified
because it is optional and uses a default value if not set. My understanding
is that the flag should, actually, always be set here.
As best as I can tell, the existing check is a redundant validation that
cannot fail and therefore is not needed. Currently the only way
s390_ipl_set_loadparm() can return non-zero is if object_property_get_str()
itself returns NULL pointer when getting the loadparm; however, the loadparm
is already validated at this point because the string is initialized when
the loadparm property is set. If there were a problem with the loadparm
string an error would have already been thrown during initialization.
Furthermore, object_property_get_str() is no longer used here. As such,
s390_ipl_set_loadparm() is changed to a void function and the check is removed.
Ok, makes sense thanks for the explanation!
Thomas
