Re: [Qemu-devel] [PATCH 3/5] s390x/ipl: Load network boot image

[Farhan Ali <alifm@linux.vnet.ibm.com>]

On 02/24/2017 05:11 AM, Thomas Huth wrote:
> On 22.02.2017 16:01, Farhan Ali wrote:
>> Hi Thomas,
>>
>> Thanks for the review.
>>
>> On 02/20/2017 10:28 AM, Thomas Huth wrote:
>>> On 20.02.2017 15:19, Cornelia Huck wrote:
>>>> From: Farhan Ali <alifm@linux.vnet.ibm.com>
> [...]
>>>> +    }
>>>> +
>>>> +    ram_ptr = memory_region_get_ram_ptr(mr);
>>>> +    if (!ram_ptr) {
>>>> +        error_report("No RAM found");
>>>> +        goto unref_mr;
>>>> +    }
>>>> +
>>>> +    netboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
>>>> ipl->netboot_fw);
>>>> +    if (netboot_filename == NULL) {
>>>> +        error_report("Could not find network bootloader");
>>>> +        goto unref_mr;
>>>> +    }
>>>
>>> So you're doing error_report() here already ...
>>>
>>>> +    img_size = load_elf_ram(netboot_filename, NULL, NULL,
>>>> &ipl->start_addr,
>>>> +                            NULL, NULL, 1, EM_S390, 0, 0, NULL, false);
>>>> +
>>>> +    if (img_size < 0) {
>>>> +        img_size = load_image_size(netboot_filename, ram_ptr,
>>>> ram_size);
>>>> +        ipl->start_addr = KERN_IMAGE_START;
>>>> +    }
>>>> +
>>>> +    g_free(netboot_filename);
>>>> +
>>>> +unref_mr:
>>>> +    memory_region_unref(mr);
>>>> +out:
>>>> +    return img_size;
>>>> +}
>>>> +
>>>> +static bool is_virtio_net_device(IplParameterBlock *iplb)
>>>> +{
>>>> +    uint8_t cssid;
>>>> +    uint8_t ssid;
>>>> +    uint16_t devno;
>>>> +    uint16_t schid;
>>>> +    SubchDev *sch = NULL;
>>>> +
>>>> +    if (iplb->pbt != S390_IPL_TYPE_CCW) {
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    devno = be16_to_cpu(iplb->ccw.devno);
>>>> +    ssid = iplb->ccw.ssid & 3;
>>>> +
>>>> +    for (schid = 0; schid < MAX_SCHID; schid++) {
>>>> +        for (cssid = 0; cssid < MAX_CSSID; cssid++) {
>>>> +            sch = css_find_subch(1, cssid, ssid, schid);
>>>> +
>>>> +            if (sch && sch->devno == devno) {
>>>> +                return sch->id.cu_model == VIRTIO_ID_NET;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +   return false;
>>>> +}
>>>> +
>>>>  void s390_ipl_update_diag308(IplParameterBlock *iplb)
>>>>  {
>>>>      S390IPLState *ipl = get_ipl_device();
>>>>
>>>>      ipl->iplb = *iplb;
>>>>      ipl->iplb_valid = true;
>>>> +    ipl->netboot = is_virtio_net_device(iplb);
>>>>  }
>>>>
>>>>  IplParameterBlock *s390_ipl_get_iplb(void)
>>>> @@ -298,6 +377,13 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>>>>              ipl->iplb_valid = s390_gen_initial_iplb(ipl);
>>>>          }
>>>>      }
>>>> +    if (ipl->netboot) {
>>>> +        if (load_netboot_image() < 0) {
>>>> +            error_report("Failed to load network bootloader");
>>>
>>> ... and then you do another error_report here again ... so one error
>>> gets reported with two error message. Wouldn't it be nicer to rather do
>>> error_setg(...) in load_netboot_image() and then report only one error
>>> at this level here?
>>>
>>
>> What would be the advantage of doing that?
>
> It's just good coding style to report an error only once, at the
> outermost calling function. Otherwise the same error gets reported
> multiple times to the user, with different error messages, and that can
> easily get confusing. It's likely not a big problem here yet, since the
> call depths is only 2 functions, but imagine a situation where you've
> got a call depth or 5 or more and an error is reported at every
> depths... that's ugly. So this is why we've got error_setg() and friends
> in QEMU.
>
>  Thomas
>

Yes, I realized it. We update with a version 2. Would appreciate your 
feedback on it :)

Thanks
Farhan