|
From: | Jared Rossi |
Subject: | Re: [PATCH 2/5] s390x: Add loadparm to CcwDevice |
Date: | Tue, 4 Jun 2024 12:27:02 -0400 |
User-agent: | Mozilla Thunderbird |
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
[Prev in Thread] | Current Thread | [Next in Thread] |