qemu-s390x
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/5] s390x: Add loadparm to CcwDevice


From: Thomas Huth
Subject: Re: [PATCH 2/5] s390x: Add loadparm to CcwDevice
Date: Tue, 4 Jun 2024 18:59:18 +0200
User-agent: Mozilla Thunderbird

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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]