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

[Thomas Huth]

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