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.
Regards,
Jared Rossi