2021-05-12 07:03:35

by Luis Chamberlain

Subject: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.

Signed-off-by: Luis Chamberlain <[email protected]>
---
drivers/block/null_blk/main.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 5f006d9e1472..2346d1292b26 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)

static int null_gendisk_register(struct nullb *nullb)
{
+ int ret;
sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
struct gendisk *disk;

@@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);

if (nullb->dev->zoned) {
- int ret = null_register_zoned_dev(nullb);
+ ret = null_register_zoned_dev(nullb);

if (ret)
return ret;
}

- add_disk(disk);
+ ret = add_disk(disk);
+ if (ret) {
+ put_disk(disk);
+ return ret;
+ }
return 0;
}

--
2.30.2

2021-05-12 15:42:49

by Hannes Reinecke

Subject: Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> We never checked for errors on add_disk() as this function
> returned void. Now that this is fixed, use the shiny new
> error handling.
>
> Signed-off-by: Luis Chamberlain <[email protected]>
> ---
> drivers/block/null_blk/main.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 5f006d9e1472..2346d1292b26 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
>
> static int null_gendisk_register(struct nullb *nullb)
> {
> + int ret;
> sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
> struct gendisk *disk;
>
> @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
> strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
>
> if (nullb->dev->zoned) {
> - int ret = null_register_zoned_dev(nullb);
> + ret = null_register_zoned_dev(nullb);
>
> if (ret)
> return ret;
> }
>
> - add_disk(disk);
> + ret = add_disk(disk);
> + if (ret) {

unregister_zoned_device() ?

> + put_disk(disk);
> + return ret;
> + }
> return 0;
> }
>
> Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

2021-05-12 19:27:55

by Luis Chamberlain

Subject: Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

On Wed, May 12, 2021 at 05:16:39PM +0200, Hannes Reinecke wrote:
> On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> > We never checked for errors on add_disk() as this function
> > returned void. Now that this is fixed, use the shiny new
> > error handling.
> >
> > Signed-off-by: Luis Chamberlain <[email protected]>
> > ---
> > drivers/block/null_blk/main.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> > index 5f006d9e1472..2346d1292b26 100644
> > --- a/drivers/block/null_blk/main.c
> > +++ b/drivers/block/null_blk/main.c
> > @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
> > static int null_gendisk_register(struct nullb *nullb)
> > {
> > + int ret;
> > sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
> > struct gendisk *disk;
> > @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
> > strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
> > if (nullb->dev->zoned) {
> > - int ret = null_register_zoned_dev(nullb);
> > + ret = null_register_zoned_dev(nullb);
> > if (ret)
> > return ret;
> > }
> > - add_disk(disk);
> > + ret = add_disk(disk);
> > + if (ret) {
>
> unregister_zoned_device() ?

That function does not exist, do you mean null_free_zoned_dev()? If so
that is done by the caller.

Luis

2021-05-12 19:45:55

by Hannes Reinecke

Subject: Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

On 5/12/21 6:47 PM, Luis Chamberlain wrote:
> On Wed, May 12, 2021 at 05:16:39PM +0200, Hannes Reinecke wrote:
>> On 5/12/21 8:46 AM, Luis Chamberlain wrote:
>>> We never checked for errors on add_disk() as this function
>>> returned void. Now that this is fixed, use the shiny new
>>> error handling.
>>>
>>> Signed-off-by: Luis Chamberlain <[email protected]>
>>> ---
>>> drivers/block/null_blk/main.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>>> index 5f006d9e1472..2346d1292b26 100644
>>> --- a/drivers/block/null_blk/main.c
>>> +++ b/drivers/block/null_blk/main.c
>>> @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
>>> static int null_gendisk_register(struct nullb *nullb)
>>> {
>>> + int ret;
>>> sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
>>> struct gendisk *disk;
>>> @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
>>> strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
>>> if (nullb->dev->zoned) {
>>> - int ret = null_register_zoned_dev(nullb);
>>> + ret = null_register_zoned_dev(nullb);
>>> if (ret)
>>> return ret;
>>> }
>>> - add_disk(disk);
>>> + ret = add_disk(disk);
>>> + if (ret) {
>>
>> unregister_zoned_device() ?
>
> That function does not exist, do you mean null_free_zoned_dev()? If so
> that is done by the caller.
>
What I intended to say is that you are calling
'null_register_zoned_dev()' at one point, but don't call a cleanup
function if there is an error later in the path, leaving the caller to
guess whether null_register_zoned_dev() has been called or not.
So we should call the cleanup function here, too.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

2021-05-12 19:54:34

by Luis Chamberlain

Subject: Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

On Wed, May 12, 2021 at 07:12:03PM +0200, Hannes Reinecke wrote:
> On 5/12/21 6:47 PM, Luis Chamberlain wrote:
> > On Wed, May 12, 2021 at 05:16:39PM +0200, Hannes Reinecke wrote:
> > > On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> > > > We never checked for errors on add_disk() as this function
> > > > returned void. Now that this is fixed, use the shiny new
> > > > error handling.
> > > >
> > > > Signed-off-by: Luis Chamberlain <[email protected]>
> > > > ---
> > > > drivers/block/null_blk/main.c | 9 +++++++--
> > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> > > > index 5f006d9e1472..2346d1292b26 100644
> > > > --- a/drivers/block/null_blk/main.c
> > > > +++ b/drivers/block/null_blk/main.c
> > > > @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
> > > > static int null_gendisk_register(struct nullb *nullb)
> > > > {
> > > > + int ret;
> > > > sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
> > > > struct gendisk *disk;
> > > > @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
> > > > strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
> > > > if (nullb->dev->zoned) {
> > > > - int ret = null_register_zoned_dev(nullb);
> > > > + ret = null_register_zoned_dev(nullb);
> > > > if (ret)
> > > > return ret;
> > > > }
> > > > - add_disk(disk);
> > > > + ret = add_disk(disk);
> > > > + if (ret) {
> > >
> > > unregister_zoned_device() ?
> >
> > That function does not exist, do you mean null_free_zoned_dev()? If so
> > that is done by the caller.
> >
> What I intended to say is that you are calling 'null_register_zoned_dev()'
> at one point, but don't call a cleanup function if there is an error later
> in the path, leaving the caller to guess whether null_register_zoned_dev()
> has been called or not.
> So we should call the cleanup function here, too.

The cleanup for zone stuff is done on the caller.

Luis

2021-05-12 19:58:04

by Hannes Reinecke

Subject: Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

On 5/12/21 7:20 PM, Luis Chamberlain wrote:
> On Wed, May 12, 2021 at 07:12:03PM +0200, Hannes Reinecke wrote:
>> On 5/12/21 6:47 PM, Luis Chamberlain wrote:
>>> On Wed, May 12, 2021 at 05:16:39PM +0200, Hannes Reinecke wrote:
>>>> On 5/12/21 8:46 AM, Luis Chamberlain wrote:
>>>>> We never checked for errors on add_disk() as this function
>>>>> returned void. Now that this is fixed, use the shiny new
>>>>> error handling.
>>>>>
>>>>> Signed-off-by: Luis Chamberlain <[email protected]>
>>>>> ---
>>>>> drivers/block/null_blk/main.c | 9 +++++++--
>>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>>>>> index 5f006d9e1472..2346d1292b26 100644
>>>>> --- a/drivers/block/null_blk/main.c
>>>>> +++ b/drivers/block/null_blk/main.c
>>>>> @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
>>>>> static int null_gendisk_register(struct nullb *nullb)
>>>>> {
>>>>> + int ret;
>>>>> sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
>>>>> struct gendisk *disk;
>>>>> @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
>>>>> strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
>>>>> if (nullb->dev->zoned) {
>>>>> - int ret = null_register_zoned_dev(nullb);
>>>>> + ret = null_register_zoned_dev(nullb);
>>>>> if (ret)
>>>>> return ret;
>>>>> }
>>>>> - add_disk(disk);
>>>>> + ret = add_disk(disk);
>>>>> + if (ret) {
>>>>
>>>> unregister_zoned_device() ?
>>>
>>> That function does not exist, do you mean null_free_zoned_dev()? If so
>>> that is done by the caller.
>>>
>> What I intended to say is that you are calling 'null_register_zoned_dev()'
>> at one point, but don't call a cleanup function if there is an error later
>> in the path, leaving the caller to guess whether null_register_zoned_dev()
>> has been called or not.
>> So we should call the cleanup function here, too.
>
> The cleanup for zone stuff is done on the caller.
>
My point being: how does he know?
The zone stuff might or might not be initialized.
Why not do the cleanup action in the error path of this function?

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

2021-05-19 20:25:34

by Luis Chamberlain

Subject: Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

On Wed, May 12, 2021 at 07:28:16PM +0200, Hannes Reinecke wrote:
> On 5/12/21 7:20 PM, Luis Chamberlain wrote:
> > On Wed, May 12, 2021 at 07:12:03PM +0200, Hannes Reinecke wrote:
> > > On 5/12/21 6:47 PM, Luis Chamberlain wrote:
> > > > On Wed, May 12, 2021 at 05:16:39PM +0200, Hannes Reinecke wrote:
> > > > > On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> > > > > > We never checked for errors on add_disk() as this function
> > > > > > returned void. Now that this is fixed, use the shiny new
> > > > > > error handling.
> > > > > >
> > > > > > Signed-off-by: Luis Chamberlain <[email protected]>
> > > > > > ---
> > > > > > drivers/block/null_blk/main.c | 9 +++++++--
> > > > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> > > > > > index 5f006d9e1472..2346d1292b26 100644
> > > > > > --- a/drivers/block/null_blk/main.c
> > > > > > +++ b/drivers/block/null_blk/main.c
> > > > > > @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
> > > > > > static int null_gendisk_register(struct nullb *nullb)
> > > > > > {
> > > > > > + int ret;
> > > > > > sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
> > > > > > struct gendisk *disk;
> > > > > > @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
> > > > > > strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
> > > > > > if (nullb->dev->zoned) {
> > > > > > - int ret = null_register_zoned_dev(nullb);
> > > > > > + ret = null_register_zoned_dev(nullb);
> > > > > > if (ret)
> > > > > > return ret;
> > > > > > }
> > > > > > - add_disk(disk);
> > > > > > + ret = add_disk(disk);
> > > > > > + if (ret) {
> > > > >
> > > > > unregister_zoned_device() ?
> > > >
> > > > That function does not exist, do you mean null_free_zoned_dev()? If so
> > > > that is done by the caller.
> > > >
> > > What I intended to say is that you are calling 'null_register_zoned_dev()'
> > > at one point, but don't call a cleanup function if there is an error later
> > > in the path, leaving the caller to guess whether null_register_zoned_dev()
> > > has been called or not.
> > > So we should call the cleanup function here, too.
> >
> > The cleanup for zone stuff is done on the caller.
> >
> My point being: how does he know?

The person who is maintainer of the code would.

> The zone stuff might or might not be initialized.
> Why not do the cleanup action in the error path of this function?

Because its not currently allocated in that function, its done in
the caller function.

Luis