[PATCH] nvme-cli: close fd before return

[PATCH] nvme-cli: close fd before return

[Chaitanya Kulkarni]

Instead of returning on the erro use existing close_fd label before
we return in create_ns() and format().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 nvme.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/nvme.c b/nvme.c
index e0f7552..d6e5b21 100644
--- a/nvme.c
+++ b/nvme.c
@@ -1216,7 +1216,7 @@ static int create_ns(int argc, char **argv, struct command *cmd, struct plugin *
 				fprintf(stderr, "identify failed\n");
 				show_nvme_status(err);
 			}
-			return err;
+			goto close_fd;
 		}
 		for (i = 0; i < 16; ++i) {
 			if ((1 << ns.lbaf[i].ds) == cfg.bs && ns.lbaf[i].ms == 0) {
@@ -1245,6 +1245,7 @@ static int create_ns(int argc, char **argv, struct command *cmd, struct plugin *
 	else
 		perror("create namespace");
 
+ close_fd:
 	close(fd);
 
 	return err;
@@ -3198,7 +3199,7 @@ static int format(int argc, char **argv, struct command *cmd, struct plugin *plu
 				fprintf(stderr, "identify failed\n");
 				show_nvme_status(err);
 			}
-			return err;
+			goto close_fd;
 		}
 		prev_lbaf = ns.flbas & 0xf;
 
-- 
2.17.0

[PATCH] nvme-cli: close fd before return

[Minwoo Im]

This kind of patches are already in Github PR:
  https://github.com/linux-nvme/nvme-cli/pull/490

> @@ -1216,7 +1216,7 @@ static int create_ns(int argc, char **argv, struct command *cmd, struct plugin *
>  				fprintf(stderr, "identify failed\n");
>  				show_nvme_status(err);
>  			}
> -			return err;
> +			goto close_fd;
>  		}
>  		for (i = 0; i < 16; ++i) {
>  			if ((1 << ns.lbaf[i].ds) == cfg.bs && ns.lbaf[i].ms == 0) {
> @@ -1245,6 +1245,7 @@ static int create_ns(int argc, char **argv, struct command *cmd, struct plugin *
>  	else
>  		perror("create namespace");
>  
> + close_fd:
>  	close(fd);
>  
>  	return err;

If this patch wants to free the leaked file descriptor in case of
errors, it needs to cover more parts than it shows above.
You can see the patch for the create_ns() from:
  https://github.com/linux-nvme/nvme-cli/pull/490/commits/b0c8a309266c2daf6ebadf9ab14884c6954765a1

> @@ -3198,7 +3199,7 @@ static int format(int argc, char **argv, struct command *cmd, struct plugin *plu
>  				fprintf(stderr, "identify failed\n");
>  				show_nvme_status(err);
>  			}
> -			return err;
> +			goto close_fd;
>  		}
>  		prev_lbaf = ns.flbas & 0xf;

It also needs to be coverted with more parts to close the leaked file
descriptor.
You can see the patch for the format() from:
  https://github.com/linux-nvme/nvme-cli/pull/490/commits/e3c487c6d9c145ba0b90d4ef227510b4faa33e98

[PATCH] nvme-cli: close fd before return

[Chaitanya Kulkarni]

This only covers the code path for nvme commands failure and not the 
errno codes ans thi ps mainly to make all the nvme error and return 
codes consistent.

On 5/19/19 11:46 AM, Minwoo Im wrote:
> This kind of patches are already in Github PR:
>    https://github.com/linux-nvme/nvme-cli/pull/490
> 
>> @@ -1216,7 +1216,7 @@ static int create_ns(int argc, char **argv, struct command *cmd, struct plugin *
>>   				fprintf(stderr, "identify failed\n");
>>   				show_nvme_status(err);
>>   			}
>> -			return err;
>> +			goto close_fd;
>>   		}
>>   		for (i = 0; i < 16; ++i) {
>>   			if ((1 << ns.lbaf[i].ds) == cfg.bs && ns.lbaf[i].ms == 0) {
>> @@ -1245,6 +1245,7 @@ static int create_ns(int argc, char **argv, struct command *cmd, struct plugin *
>>   	else
>>   		perror("create namespace");
>>   
>> + close_fd:
>>   	close(fd);
>>   
>>   	return err;
> 
> If this patch wants to free the leaked file descriptor in case of
> errors, it needs to cover more parts than it shows above.
> You can see the patch for the create_ns() from:
>    https://github.com/linux-nvme/nvme-cli/pull/490/commits/b0c8a309266c2daf6ebadf9ab14884c6954765a1
> 
>> @@ -3198,7 +3199,7 @@ static int format(int argc, char **argv, struct command *cmd, struct plugin *plu
>>   				fprintf(stderr, "identify failed\n");
>>   				show_nvme_status(err);
>>   			}
>> -			return err;
>> +			goto close_fd;
>>   		}
>>   		prev_lbaf = ns.flbas & 0xf;
> 
> It also needs to be coverted with more parts to close the leaked file
> descriptor.
> You can see the patch for the format() from:
>    https://github.com/linux-nvme/nvme-cli/pull/490/commits/e3c487c6d9c145ba0b90d4ef227510b4faa33e98
> 

[PATCH] nvme-cli: close fd before return

[Minwoo Im]

> This only covers the code path for nvme commands failure and not the
> errno codes ans thi ps mainly to make all the nvme error and return
> codes consistent.

I think I don't agree with that.  If you want to fix the leaked fd before
returning directly, then it should make it go to close_fd for the EINVAL
cases without adding -(negative) on it.

I think the PR can be merged regardless to errno update now, though.

[PATCH] nvme-cli: close fd before return

[Chaitanya Kulkarni]

On 5/19/19 5:28 PM, Minwoo Im wrote:
>> This only covers the code path for nvme commands failure and not the
>> errno codes ans thi ps mainly to make all the nvme error and return
>> codes consistent.
> 
> I think I don't agree with that.  If you want to fix the leaked fd before
> returning directly, then it should make it go to close_fd for the EINVAL
> cases without adding -(negative) on it.
> 
> I think the PR can be merged regardless to errno update now, though.
> 

Before we add your patch-series we need to make all the returns 
consistent which is not present in the format() and create_ns() for nvme 
error status. Ofcourse eventually all the fd leaks needs to be fixed.
This patch makes it sure the leaks for the only nvme error code are 
consistent and on the top of that your series fits well.

Without this we need to add two return calls in the create_ns () 
http://lists.infradead.org/pipermail/linux-nvme/2019-May/024307.html :-

static int attach_ns(int argc, char **argv, struct command *cmd, struct 
plugin *plugin)
@@ -1216,7 +1210,7 @@ static int create_ns(int argc, char **argv, struct 
command *cmd, struct plugin *
  				fprintf(stderr, "identify failed\n");
  				show_nvme_status(err);
  			}
-			return err;
+			return nvme_status_to_errno(err, false);
  		}
  		for (i = 0; i < 16; ++i) {
  			if ((1 << ns.lbaf[i].ds) == cfg.bs && ns.lbaf[i].ms == 0) {
@@ -1246,8 +1240,7 @@ static int create_ns(int argc, char **argv, struct 
command *cmd, struct plugin *
  		perror("create namespace");

  	close(fd);
-
-	return err;
+	return nvme_status_to_errno(err, false);
  }

It should be consistent with all the over functions with one return.

This what I'm trying to avoid (by ignoring other leaks for now), can you 
please explain why it is not a good idea to avoid this ?

[PATCH] nvme-cli: close fd before return

[Minwoo Im]

> Before we add your patch-series we need to make all the returns
> consistent which is not present in the format() and create_ns() for nvme
> error status. Ofcourse eventually all the fd leaks needs to be fixed.
> This patch makes it sure the leaks for the only nvme error code are
> consistent and on the top of that your series fits well.
> 
> Without this we need to add two return calls in the create_ns ()
> http://lists.infradead.org/pipermail/linux-nvme/2019-May/024307.html :-
> 
> static int attach_ns(int argc, char **argv, struct command *cmd, struct
> plugin *plugin)
> @@ -1216,7 +1210,7 @@ static int create_ns(int argc, char **argv, struct
> command *cmd, struct plugin *
>   				fprintf(stderr, "identify failed\n");
>   				show_nvme_status(err);
>   			}
> -			return err;
> +			return nvme_status_to_errno(err, false);
>   		}
>   		for (i = 0; i < 16; ++i) {
>   			if ((1 << ns.lbaf[i].ds) == cfg.bs && ns.lbaf[i].ms == 0)
> {
> @@ -1246,8 +1240,7 @@ static int create_ns(int argc, char **argv, struct
> command *cmd, struct plugin *
>   		perror("create namespace");
> 
>   	close(fd);
> -
> -	return err;
> +	return nvme_status_to_errno(err, false);
>   }
> 
> It should be consistent with all the over functions with one return.
> 
> This what I'm trying to avoid (by ignoring other leaks for now), can you
> please explain why it is not a good idea to avoid this ?

I never thought it was not a good idea.  I just told you that it's just a
duplicated patch with on-going PR in the Github.

If we are returning the nvme_status_to_errno() in that other patch that
I have posted twice, is it that critical factor which blocks the patchsets to
be merged?

I don't know what exactly the issues are between "first merge patch serias
and code-cleanup there",  and "first merge this fix and rebase the patch series".
When I was preparing the patch series about errno, I was also hate that
codes you have posted above, but I need to make it focused first to make
people review this series first even there are two times calling like above.

If you really want to make it consistent, then why don't we just title it like
"Do not return in case of errors in the middle"

If you really want to close fd to fix the leak issue, then I think it needs to 
cover more than that.

[PATCH] nvme-cli: close fd before return

[Chaitanya Kulkarni]

On 5/19/19 6:54 PM, Minwoo Im wrote:
>> Before we add your patch-series we need to make all the returns
>> consistent which is not present in the format() and create_ns() for nvme
>> error status. Ofcourse eventually all the fd leaks needs to be fixed.
>> This patch makes it sure the leaks for the only nvme error code are
>> consistent and on the top of that your series fits well.
>>
>> Without this we need to add two return calls in the create_ns ()
>> http://lists.infradead.org/pipermail/linux-nvme/2019-May/024307.html :-
>>
>> static int attach_ns(int argc, char **argv, struct command *cmd, struct
>> plugin *plugin)
>> @@ -1216,7 +1210,7 @@ static int create_ns(int argc, char **argv, struct
>> command *cmd, struct plugin *
>>   				fprintf(stderr, "identify failed\n");
>>   				show_nvme_status(err);
>>   			}
>> -			return err;
>> +			return nvme_status_to_errno(err, false);
>>   		}
>>   		for (i = 0; i < 16; ++i) {
>>   			if ((1 << ns.lbaf[i].ds) == cfg.bs && ns.lbaf[i].ms == 0)
>> {
>> @@ -1246,8 +1240,7 @@ static int create_ns(int argc, char **argv, struct
>> command *cmd, struct plugin *
>>   		perror("create namespace");
>>
>>   	close(fd);
>> -
>> -	return err;
>> +	return nvme_status_to_errno(err, false);
>>   }
>>
>> It should be consistent with all the over functions with one return.
>>
>> This what I'm trying to avoid (by ignoring other leaks for now), can you
>> please explain why it is not a good idea to avoid this ?
> I never thought it was not a good idea.  I just told you that it's just a
> duplicated patch with on-going PR in the Github.

I've not said anything against PR. I was just explaining the idea behind
sending

this patch. We can leave it to PR now.

But this is a good example of having things not be posted on the mailing
list can

a create confusion, will be monitoring PR going forward.

>
> If we are returning the nvme_status_to_errno() in that other patch that
> I have posted twice, is it that critical factor which blocks the patchsets to
> be merged?
No it is not but it just makes your series smooth.
>
> I don't know what exactly the issues are between "first merge patch serias
> and code-cleanup there",  and "first merge this fix and rebase the patch series".
> When I was preparing the patch series about errno, I was also hate that
> codes you have posted above, 
This is exactly my intent behind this patch.
> but I need to make it focused first to make
> people review this series first even there are two times calling like above.

So far I've not encountered situation where PRs are being discussed on
github and

patch-series is not posted on the mailing list, but it clear to me it
can create a confusion

as some people are not following github as closely as mailing list and
we don't have

the clarity over which has the priority when it comes to sending patches
whether it is github or

the NVMe mailing list.

>From my side, I'll make sure to monitor the PR on github before posting
any further patches.

> If you really want to make it consistent, then why don't we just title it like
> "Do not return in case of errors in the middle"

I don't mind that in fact I can resend this patch refer to your
patch-set and mark it

as a preparation patch with changing title, if PR wasn't in process.

>
> If you really want to close fd to fix the leak issue, then I think it needs to 
> cover more than that.
I've never opposed to fix the leak, but this was not the intent.

As far as this patch goes, since you are already working on PR with
github, will retract this

change and we should go with that change.

[PATCH] nvme-cli: close fd before return

[Minwoo Im]

> I've not said anything against PR. I was just explaining the idea behind
> sending
> 
> this patch. We can leave it to PR now.
> 
> But this is a good example of having things not be posted on the mailing
> list can
> 
> a create confusion, will be monitoring PR going forward.

Agree with your point.  Actually, recently I'm trying to send patches of nvme-cli
to linux-nvme mailing list, but I'm not sure whether the PR will not be allowed or
not because I'm a just one who contributes this project :)

> So far I've not encountered situation where PRs are being discussed on
> github and
> 
> patch-series is not posted on the mailing list, but it clear to me it
> can create a confusion
> 
> as some people are not following github as closely as mailing list and
> we don't have
> 
> the clarity over which has the priority when it comes to sending patches
> whether it is github or
> 
> the NVMe mailing list.
> 
> From my side, I'll make sure to monitor the PR on github before posting
> any further patches.

Thanks for your support, But I know it's hard to monitor Github PRs all the time.

> I don't mind that in fact I can resend this patch refer to your
> patch-set and mark it
> 
> as a preparation patch with changing title, if PR wasn't in process.

Actually, I'm okay with your patch by making it focused onto something clear.
Anyway, this patch looks good to me in code level, though.

> I've never opposed to fix the leak, but this was not the intent.
> 
> As far as this patch goes, since you are already working on PR with
> github, will retract this
> 
> change and we should go with that change.

Let's see how we can go through it together :)