bluez: btmgmt --index broken

bluez: btmgmt --index broken

[Juerg Haefliger]

[-- Attachment #1: Type: text/plain, Size: 2255 bytes --]

Hi,

Commit 648b4362521b ("shared/shell: Add support for -i/--init-script")
introduced a short option namespace collision with btmgmt's --index option,
both of which use '-i'.

As a result, a provided --index is treated as an init-script...

$ sudo btmgmt --index 0 info
Unable to open 0: No such file or directory (2)


On a separate note, should btmgmt actually support --init-script since it
doesn't seem to do anything and just enters interactive mode?

$ cat test
help
info
quit

$ btmgmt --init-script test
[mgmt]#


Simple fix for the first issue, use -s/--init-script instead:

diff --git a/src/shared/shell.c b/src/shared/shell.c
index db79c882c..fbccff5b5 100644
--- a/src/shared/shell.c
+++ b/src/shared/shell.c
@@ -1128,7 +1128,7 @@ static void rl_init(void)
 static const struct option main_options[] = {
        { "version",    no_argument, 0, 'v' },
        { "help",       no_argument, 0, 'h' },
-       { "init-script", required_argument, 0, 'i' },
+       { "init-script", required_argument, 0, 's' },
        { "timeout",    required_argument, 0, 't' },
        { "monitor",    no_argument, 0, 'm' },
        { "zsh-complete",       no_argument, 0, 'z' },
@@ -1169,9 +1169,9 @@ void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt)
        if (opt) {
                memcpy(options + offset, opt->options,
                                sizeof(struct option) * opt->optno);
-               snprintf(optstr, sizeof(optstr), "+mhvi:t:%s", opt->optstr);
+               snprintf(optstr, sizeof(optstr), "+mhvs:t:%s", opt->optstr);
        } else
-               snprintf(optstr, sizeof(optstr), "+mhvi:t:");
+               snprintf(optstr, sizeof(optstr), "+mhvs:t:");
 
        data.name = strrchr(argv[0], '/');
        if (!data.name)
@@ -1193,7 +1193,7 @@ void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt)
                        data.argv = &cmplt;
                        data.mode = 1;
                        goto done;
-               case 'i':
+               case 's':
                        if (optarg)
                                data.init_fd = open(optarg, O_RDONLY);
                        if (data.init_fd < 0)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: bluez: btmgmt --index broken

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: src/shared/shell.c:1128
error: src/shared/shell.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

[BlueZ PATCH] shared/shell: Fix --init-script commandline option

[Juerg Haefliger]

The newly added option -i/--init-script introduced a short option
namespace collision with btmgmt's --index, both of which use '-i'.

As a result, a provided --index is treated as a file name:

$ sudo btmgmt --index 0 info
Unable to open 0: No such file or directory (2)

Fix this by using '-s' for --init-script.

Fixes: f2f7c742ad0b ("shared/shell: Add support for -i/--init-script")
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
---
 src/shared/shell.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/shared/shell.c b/src/shared/shell.c
index db79c882ca3a..fbccff5b54d9 100644
--- a/src/shared/shell.c
+++ b/src/shared/shell.c
@@ -1128,7 +1128,7 @@ static void rl_init(void)
 static const struct option main_options[] = {
 	{ "version",	no_argument, 0, 'v' },
 	{ "help",	no_argument, 0, 'h' },
-	{ "init-script", required_argument, 0, 'i' },
+	{ "init-script", required_argument, 0, 's' },
 	{ "timeout",	required_argument, 0, 't' },
 	{ "monitor",	no_argument, 0, 'm' },
 	{ "zsh-complete",	no_argument, 0, 'z' },
@@ -1169,9 +1169,9 @@ void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt)
 	if (opt) {
 		memcpy(options + offset, opt->options,
 				sizeof(struct option) * opt->optno);
-		snprintf(optstr, sizeof(optstr), "+mhvi:t:%s", opt->optstr);
+		snprintf(optstr, sizeof(optstr), "+mhvs:t:%s", opt->optstr);
 	} else
-		snprintf(optstr, sizeof(optstr), "+mhvi:t:");
+		snprintf(optstr, sizeof(optstr), "+mhvs:t:");
 
 	data.name = strrchr(argv[0], '/');
 	if (!data.name)
@@ -1193,7 +1193,7 @@ void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt)
 			data.argv = &cmplt;
 			data.mode = 1;
 			goto done;
-		case 'i':
+		case 's':
 			if (optarg)
 				data.init_fd = open(optarg, O_RDONLY);
 			if (data.init_fd < 0)
-- 
2.39.2

RE: [BlueZ] shared/shell: Fix --init-script commandline option

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=797007

---Test result---

Test Summary:
CheckPatch                    FAIL      0.75 seconds
GitLint                       PASS      0.37 seconds
BuildEll                      PASS      34.28 seconds
BluezMake                     PASS      998.57 seconds
MakeCheck                     PASS      13.63 seconds
MakeDistcheck                 PASS      204.73 seconds
CheckValgrind                 PASS      320.96 seconds
CheckSmatch                   WARNING   423.94 seconds
bluezmakeextell               PASS      141.24 seconds
IncrementalBuild              PASS      838.70 seconds
ScanBuild                     PASS      1271.33 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ] shared/shell: Fix --init-script commandline option
WARNING:UNKNOWN_COMMIT_ID: Unknown commit id 'f2f7c742ad0b', maybe rebased or not pulled?
#105: 
Fixes: f2f7c742ad0b ("shared/shell: Add support for -i/--init-script")

/github/workspace/src/src/13438114.patch total: 0 errors, 1 warnings, 27 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13438114.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):


---
Regards,
Linux Bluetooth

[PATCH v2] shared/shell: Fix --init-script commandline option

[Juerg Haefliger]

The newly added option -i/--init-script introduced a short option
namespace collision with btmgmt's --index, both of which use '-i'.

As a result, a provided --index is treated as a file name:

$ sudo btmgmt --index 0 info
Unable to open 0: No such file or directory (2)

Fix this by using '-s' for --init-script.

Fixes: https://github.com/bluez/bluez/issues/639
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>

---
v2:
  - Replace reference to broken commit with reference to github issue.
---
 src/shared/shell.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/shared/shell.c b/src/shared/shell.c
index db79c882ca3a..fbccff5b54d9 100644
--- a/src/shared/shell.c
+++ b/src/shared/shell.c
@@ -1128,7 +1128,7 @@ static void rl_init(void)
 static const struct option main_options[] = {
 	{ "version",	no_argument, 0, 'v' },
 	{ "help",	no_argument, 0, 'h' },
-	{ "init-script", required_argument, 0, 'i' },
+	{ "init-script", required_argument, 0, 's' },
 	{ "timeout",	required_argument, 0, 't' },
 	{ "monitor",	no_argument, 0, 'm' },
 	{ "zsh-complete",	no_argument, 0, 'z' },
@@ -1169,9 +1169,9 @@ void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt)
 	if (opt) {
 		memcpy(options + offset, opt->options,
 				sizeof(struct option) * opt->optno);
-		snprintf(optstr, sizeof(optstr), "+mhvi:t:%s", opt->optstr);
+		snprintf(optstr, sizeof(optstr), "+mhvs:t:%s", opt->optstr);
 	} else
-		snprintf(optstr, sizeof(optstr), "+mhvi:t:");
+		snprintf(optstr, sizeof(optstr), "+mhvs:t:");
 
 	data.name = strrchr(argv[0], '/');
 	if (!data.name)
@@ -1193,7 +1193,7 @@ void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt)
 			data.argv = &cmplt;
 			data.mode = 1;
 			goto done;
-		case 'i':
+		case 's':
 			if (optarg)
 				data.init_fd = open(optarg, O_RDONLY);
 			if (data.init_fd < 0)
-- 
2.39.2

RE: [v2] shared/shell: Fix --init-script commandline option

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=797481

---Test result---

Test Summary:
CheckPatch                    PASS      0.42 seconds
GitLint                       PASS      0.29 seconds
BuildEll                      PASS      29.61 seconds
BluezMake                     PASS      951.63 seconds
MakeCheck                     PASS      13.17 seconds
MakeDistcheck                 PASS      184.63 seconds
CheckValgrind                 PASS      282.56 seconds
CheckSmatch                   WARNING   379.14 seconds
bluezmakeextell               PASS      121.60 seconds
IncrementalBuild              PASS      785.02 seconds
ScanBuild                     PASS      1164.83 seconds

Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):


---
Regards,
Linux Bluetooth

Re: [PATCH v2] shared/shell: Fix --init-script commandline option

[Luiz Augusto von Dentz]

Hi Juerg,

On Mon, Oct 30, 2023 at 2:54 AM Juerg Haefliger
<juerg.haefliger@canonical.com> wrote:
>
> The newly added option -i/--init-script introduced a short option
> namespace collision with btmgmt's --index, both of which use '-i'.
>
> As a result, a provided --index is treated as a file name:
>
> $ sudo btmgmt --index 0 info

Perhaps we could remove this --index since btmgmt supports setting the
index via select command; it doesn't seem very useful to have 2
different forms of selecting the index.

> Unable to open 0: No such file or directory (2)
>
> Fix this by using '-s' for --init-script.
>
> Fixes: https://github.com/bluez/bluez/issues/639
> Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
>
> ---
> v2:
>   - Replace reference to broken commit with reference to github issue.
> ---
>  src/shared/shell.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/shared/shell.c b/src/shared/shell.c
> index db79c882ca3a..fbccff5b54d9 100644
> --- a/src/shared/shell.c
> +++ b/src/shared/shell.c
> @@ -1128,7 +1128,7 @@ static void rl_init(void)
>  static const struct option main_options[] = {
>         { "version",    no_argument, 0, 'v' },
>         { "help",       no_argument, 0, 'h' },
> -       { "init-script", required_argument, 0, 'i' },
> +       { "init-script", required_argument, 0, 's' },
>         { "timeout",    required_argument, 0, 't' },
>         { "monitor",    no_argument, 0, 'm' },
>         { "zsh-complete",       no_argument, 0, 'z' },
> @@ -1169,9 +1169,9 @@ void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt)
>         if (opt) {
>                 memcpy(options + offset, opt->options,
>                                 sizeof(struct option) * opt->optno);
> -               snprintf(optstr, sizeof(optstr), "+mhvi:t:%s", opt->optstr);
> +               snprintf(optstr, sizeof(optstr), "+mhvs:t:%s", opt->optstr);
>         } else
> -               snprintf(optstr, sizeof(optstr), "+mhvi:t:");
> +               snprintf(optstr, sizeof(optstr), "+mhvs:t:");
>
>         data.name = strrchr(argv[0], '/');
>         if (!data.name)
> @@ -1193,7 +1193,7 @@ void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt)
>                         data.argv = &cmplt;
>                         data.mode = 1;
>                         goto done;
> -               case 'i':
> +               case 's':
>                         if (optarg)
>                                 data.init_fd = open(optarg, O_RDONLY);
>                         if (data.init_fd < 0)
> --
> 2.39.2
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v2] shared/shell: Fix --init-script commandline option

[Juerg Haefliger]

[-- Attachment #1: Type: text/plain, Size: 3205 bytes --]

Hi Luiz,

> Hi Juerg,
> 
> On Mon, Oct 30, 2023 at 2:54 AM Juerg Haefliger
> <juerg.haefliger@canonical.com> wrote:
> >
> > The newly added option -i/--init-script introduced a short option
> > namespace collision with btmgmt's --index, both of which use '-i'.
> >
> > As a result, a provided --index is treated as a file name:
> >
> > $ sudo btmgmt --index 0 info  
> 
> Perhaps we could remove this --index since btmgmt supports setting the
> index via select command; it doesn't seem very useful to have 2
> different forms of selecting the index.

That would break a potentially large number of existing scripts, which is bad.
--index has been around for a long time and is also supported by other bluez
commands. It would require some warning first that it's going away to give
people time to transition.

I'd rather fix the new option which hasn't been released yet and introduced
this regression.

...Juerg

 
> > Unable to open 0: No such file or directory (2)
> >
> > Fix this by using '-s' for --init-script.
> >
> > Fixes: https://github.com/bluez/bluez/issues/639
> > Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> >
> > ---
> > v2:
> >   - Replace reference to broken commit with reference to github issue.
> > ---
> >  src/shared/shell.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/shared/shell.c b/src/shared/shell.c
> > index db79c882ca3a..fbccff5b54d9 100644
> > --- a/src/shared/shell.c
> > +++ b/src/shared/shell.c
> > @@ -1128,7 +1128,7 @@ static void rl_init(void)
> >  static const struct option main_options[] = {
> >         { "version",    no_argument, 0, 'v' },
> >         { "help",       no_argument, 0, 'h' },
> > -       { "init-script", required_argument, 0, 'i' },
> > +       { "init-script", required_argument, 0, 's' },
> >         { "timeout",    required_argument, 0, 't' },
> >         { "monitor",    no_argument, 0, 'm' },
> >         { "zsh-complete",       no_argument, 0, 'z' },
> > @@ -1169,9 +1169,9 @@ void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt)
> >         if (opt) {
> >                 memcpy(options + offset, opt->options,
> >                                 sizeof(struct option) * opt->optno);
> > -               snprintf(optstr, sizeof(optstr), "+mhvi:t:%s", opt->optstr);
> > +               snprintf(optstr, sizeof(optstr), "+mhvs:t:%s", opt->optstr);
> >         } else
> > -               snprintf(optstr, sizeof(optstr), "+mhvi:t:");
> > +               snprintf(optstr, sizeof(optstr), "+mhvs:t:");
> >
> >         data.name = strrchr(argv[0], '/');
> >         if (!data.name)
> > @@ -1193,7 +1193,7 @@ void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt)
> >                         data.argv = &cmplt;
> >                         data.mode = 1;
> >                         goto done;
> > -               case 'i':
> > +               case 's':
> >                         if (optarg)
> >                                 data.init_fd = open(optarg, O_RDONLY);
> >                         if (data.init_fd < 0)
> > --
> > 2.39.2
> >  
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] shared/shell: Fix --init-script commandline option

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon, 30 Oct 2023 07:53:41 +0100 you wrote:
> The newly added option -i/--init-script introduced a short option
> namespace collision with btmgmt's --index, both of which use '-i'.
> 
> As a result, a provided --index is treated as a file name:
> 
> $ sudo btmgmt --index 0 info
> Unable to open 0: No such file or directory (2)
> 
> [...]

Here is the summary with links:
  - [v2] shared/shell: Fix --init-script commandline option
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=303925b28110

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html