On 4/27/23 6:26?AM, Anuj gupta wrote:
> On Thu, Apr 27, 2023 at 5:23?PM Jens Axboe <axboe@xxxxxxxxx> wrote:
>>
>> On 4/27/23 5:48?AM, Jens Axboe wrote:
>>> On 4/27/23 9:10?AM, Anuj Gupta wrote:
>>>> s->sq_ring.ring_entries and s->cq_ring.ring_entries will be NULL,
>>>> incase setup_ring fails. This will cause a segmentation fault.
>>>>
>>>> In case setup_ring fails, bail out by setting finish.
>>>
>>> Any reason why we don't just use the return code of submitter_init()
>>> on whether to abort or not?
>>
>> Something like this as a prep patch, then do the trivial version
>> of your patch after that.
>>
>> Totally untested...
>>
>>
>> diff --git a/t/io_uring.c b/t/io_uring.c
>> index 6b0efef85cb7..2f8d63fbe67e 100644
>> --- a/t/io_uring.c
>> +++ b/t/io_uring.c
>> @@ -1049,7 +1049,7 @@ static int submitter_init(struct submitter *s)
>>
>> buf = allocate_mem(s, bs);
>> if (!buf)
>> - return 1;
>> + return -1;
>> s->iovecs[i].iov_base = buf;
>> s->iovecs[i].iov_len = bs;
>> }
>> @@ -1066,7 +1066,7 @@ static int submitter_init(struct submitter *s)
>> }
>> if (err) {
>> printf("queue setup failed: %s, %d\n", strerror(errno), err);
>> - return 1;
>> + return -1;
>> }
>>
>> if (!init_printed) {
>> @@ -1172,9 +1172,15 @@ static void *submitter_aio_fn(void *data)
>> struct iocb *iocbs;
>> struct io_event *events;
>> #ifdef ARCH_HAVE_CPU_CLOCK
>> - int nr_batch = submitter_init(s);
>> -#else
>> - submitter_init(s);
>> + int nr_batch;
>> +#endif
>> +
>> + ret = submitter_init(s);
>> + if (ret < 0)
>> + goto done;
>> +
>> +#ifdef ARCH_HAVE_CPU_CLOCK
>> + nr_batch = ret;
>> #endif
>>
>> iocbsptr = calloc(depth, sizeof(struct iocb *));
>> @@ -1238,6 +1244,7 @@ static void *submitter_aio_fn(void *data)
>> free(iocbsptr);
>> free(iocbs);
>> free(events);
>> +done:
>> finish = 1;
>> return NULL;
>> }
>> @@ -1277,9 +1284,15 @@ static void *submitter_uring_fn(void *data)
>> struct io_sq_ring *ring = &s->sq_ring;
>> int ret, prepped;
>> #ifdef ARCH_HAVE_CPU_CLOCK
>> - int nr_batch = submitter_init(s);
>> -#else
>> - submitter_init(s);
>> + int nr_batch;
>> +#endif
>> +
>> + ret = submitter_init(s);
>> + if (ret < 0)
>> + goto done;
>> +
>> +#ifdef ARCH_HAVE_CPU_CLOCK
>> + nr_batch = ret;
>> #endif
>>
>> if (register_ring)
>> @@ -1383,6 +1396,7 @@ submit:
>> if (register_ring)
>> io_uring_unregister_ring(s);
>>
>> +done:
>> finish = 1;
>> return NULL;
>> }
>> @@ -1393,7 +1407,8 @@ static void *submitter_sync_fn(void *data)
>> struct submitter *s = data;
>> int ret;
>>
>> - submitter_init(s);
>> + if (submitter_init(s) < 0)
>> + goto done;
>>
>> do {
>> uint64_t offset;
>> @@ -1429,6 +1444,7 @@ static void *submitter_sync_fn(void *data)
>> add_stat(s, s->clock_index, 1);
>> } while (!s->finish);
>>
>> +done:
>> finish = 1;
>> return NULL;
>> }
>>
>
> Tested this and the patch works out fine. This can go on top of this
> to avoid access to NULL pointer -
Thanks! Can you send this one as a real patch to make my life just a
tiny bit easier?
--
Jens Axboe
