[PATCH] block/iscsi: Document why we use raw malloc()

[PATCH] block/iscsi: Document why we use raw malloc()

[Peter Maydell]

In block/iscsi.c we use a raw malloc() call, which is unusual
given the project standard is to use the glib memory allocation
functions. Document why we do so, to avoid it being converted
to g_malloc() by mistake.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
There aren't many uses of raw malloc() in the codebase
other than third-party library sourcecode. Mostly we have
a comment noting when we're doing it deliberately.
---
 block/iscsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 34f97ab6460..5640c8b5657 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1058,6 +1058,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
         return NULL;
     }
 
+    /* Must use malloc(): this is freed via scsi_free_scsi_task() */
     acb->task = malloc(sizeof(struct scsi_task));
     if (acb->task == NULL) {
         error_report("iSCSI: Failed to allocate task for scsi command. %s",
-- 
2.34.1

Re: [PATCH] block/iscsi: Document why we use raw malloc()

[Peter Maydell]

On Thu, 27 Jul 2023 at 16:07, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In block/iscsi.c we use a raw malloc() call, which is unusual
> given the project standard is to use the glib memory allocation
> functions. Document why we do so, to avoid it being converted
> to g_malloc() by mistake.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> There aren't many uses of raw malloc() in the codebase
> other than third-party library sourcecode. Mostly we have
> a comment noting when we're doing it deliberately.
> ---
>  block/iscsi.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 34f97ab6460..5640c8b5657 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1058,6 +1058,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
>          return NULL;
>      }
>
> +    /* Must use malloc(): this is freed via scsi_free_scsi_task() */
>      acb->task = malloc(sizeof(struct scsi_task));
>      if (acb->task == NULL) {
>          error_report("iSCSI: Failed to allocate task for scsi command. %s",

The other option here would be to use scsi_create_task(),
if we're OK with requiring libiscsi 1.13 or better (that's
9 years old at this point, so should be OK...)

thanks
-- PMM

Re: [PATCH] block/iscsi: Document why we use raw malloc()

[Kevin Wolf]

Am 27.07.2023 um 17:15 hat Peter Maydell geschrieben:
> On Thu, 27 Jul 2023 at 16:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > In block/iscsi.c we use a raw malloc() call, which is unusual
> > given the project standard is to use the glib memory allocation
> > functions. Document why we do so, to avoid it being converted
> > to g_malloc() by mistake.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > There aren't many uses of raw malloc() in the codebase
> > other than third-party library sourcecode. Mostly we have
> > a comment noting when we're doing it deliberately.

Thanks, applied to the block branch.

> The other option here would be to use scsi_create_task(),
> if we're OK with requiring libiscsi 1.13 or better (that's
> 9 years old at this point, so should be OK...)

I wouldn't mind either way, but since this patch exists and the other
doesn't, I'm applying this one for now.

Kevin

Re: [PATCH] block/iscsi: Document why we use raw malloc()

[Peter Maydell]

On Fri, 18 Aug 2023 at 16:56, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 27.07.2023 um 17:15 hat Peter Maydell geschrieben:
> > On Thu, 27 Jul 2023 at 16:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > In block/iscsi.c we use a raw malloc() call, which is unusual
> > > given the project standard is to use the glib memory allocation
> > > functions. Document why we do so, to avoid it being converted
> > > to g_malloc() by mistake.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > > There aren't many uses of raw malloc() in the codebase
> > > other than third-party library sourcecode. Mostly we have
> > > a comment noting when we're doing it deliberately.
>
> Thanks, applied to the block branch.
>
> > The other option here would be to use scsi_create_task(),
> > if we're OK with requiring libiscsi 1.13 or better (that's
> > 9 years old at this point, so should be OK...)
>
> I wouldn't mind either way, but since this patch exists and the other
> doesn't, I'm applying this one for now.

Thanks. I think scsi_create_task() would be cleaner, but
it's just over the amount of change that I'd want to do without
a test setup to exercise this code, which is why I stuck to
the comment for now.

-- PMM