> On 13 Sep 2016, at 14:20 PM, P J P <ppan...@redhat.com> wrote:
>
> Hello Dmitry,
>
> +-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
> | Better put here:
> |
> | if(elemcnt++ >= PVSCSI_MAX_SG_ELEM) {
> | return;
> | }
> |
> | And ditch additional conditions from while clauses.
> | This way you’ll have less compare operations and avoid
> | adding the same element twice when leaving because of too long SG list.
>
> Okay.
>
> | Also this is not a good idea to send truncated requests
> | when SG list becomes too long. I’d prefer to return error
> | code from pvscsi_convert_sglist() and handle it as appropriate.
>
> This could be a separate patch. Should pvscsi_build_sglist() destroy the
> sglist on error?
It should be in the same patch because otherwise a broken logic will be
introduced.
pvscsi_build_sglist() must perform all rollback actions required to handle the
error cleanly.
Please also note that pvscsi_build_sglist() caller(s) should be aware of SG
list creation failure
and handle this error scenario accordingly.
>
> @@ -656,7 +660,9 @@ pvscsi_build_sglist(PVSCSIState *s, PVSCSIRequest *r)
>
> pci_dma_sglist_init(&r->sgl, d, 1);
> if (r->req.flags & PVSCSI_FLAG_CMD_WITH_SG_LIST) {
> - pvscsi_convert_sglist(r);
> + if (pvscsi_convert_sglist(r) < 0) {
> + qemu_sglist_destroy(&r->sgl);
> + }
> } else {
> qemu_sglist_add(&r->sgl, r->req.dataAddr, r->req.dataLen);
> }
>
> I'll send both patches together once you confirm.
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
