You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/12/11 16:13:54 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #4978: libc: Add getprogname function
xiaoxiang781216 opened a new pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978
## Summary
defined here:
https://www.freebsd.org/cgi/man.cgi?query=getprogname&sektion=3
## Impact
New API
## Testing
Pass CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#discussion_r767218079
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: getprogname
+ ****************************************************************************/
+
+FAR const char *getprogname(void)
+{
+ FAR struct task_info_s *info;
+
+ info = task_get_info();
+ return info->argv[0];
Review comment:
We don't need check CONFIG_BUILD_KERNEL here, since argv field always exist in all three modes(flat, protected and kernel).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-994880835
@pkarashchenko do you think this patch ready for merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-991829467
> In general current approach seems to be fine, but most probably adding `setprogname` will require adding additional field into `struct task_info_s`.
>
setprogname is normally used by glibc internally, to initialize the program name. But the program name is initialized automatically in NuttX, so setprogname isn't really needed here.
> BWT, maybe you can also fix a small bug in `gettextdomain` implementation by changing `#ifdef CONFIG_BUILD_KERNEL` to `#ifndef CONFIG_BUILD_KERNEL`. I just found it today during the code review.
Fixed here: https://github.com/apache/incubator-nuttx/pull/4982
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-991829467
> In general current approach seems to be fine, but most probably adding `setprogname` will require adding additional field into `struct task_info_s`.
>
setprogname is normally used by glibc internally, to initialize the program name. But the program name is initialized automatically in NuttX, so setprogname isn't really needed here.
> BWT, maybe you can also fix a small bug in `gettextdomain` implementation by changing `#ifdef CONFIG_BUILD_KERNEL` to `#ifndef CONFIG_BUILD_KERNEL`. I just found it today during the code review.
Fixed here: https://github.com/apache/incubator-nuttx/pull/4982
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#discussion_r767218079
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: getprogname
+ ****************************************************************************/
+
+FAR const char *getprogname(void)
+{
+ FAR struct task_info_s *info;
+
+ info = task_get_info();
+ return info->argv[0];
Review comment:
We don't check CONFIG_BUILD_KERNEL here, since argv field always exist in all three mode(flat, protected and kernel).
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: getprogname
+ ****************************************************************************/
+
+FAR const char *getprogname(void)
+{
+ FAR struct task_info_s *info;
+
+ info = task_get_info();
+ return info->argv[0];
Review comment:
We don't need check CONFIG_BUILD_KERNEL here, since argv field always exist in all three mode(flat, protected and kernel).
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
Review comment:
Done.
##########
File path: include/stdlib.h
##########
@@ -265,6 +265,8 @@ FAR void *bsearch(FAR const void *key, FAR const void *base, size_t nel,
size_t width, CODE int (*compar)(FAR const void *,
FAR const void *));
+FAR const char *getprogname(void);
Review comment:
Done.
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: getprogname
+ ****************************************************************************/
+
+FAR const char *getprogname(void)
+{
+ FAR struct task_info_s *info;
+
+ info = task_get_info();
+ return info->argv[0];
Review comment:
We don't need check CONFIG_BUILD_KERNEL here, since argv field always exist in all three modes(flat, protected and kernel).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-994883558
> @pkarashchenko do you think this patch ready for merge?
@xiaoxiang781216 Yes. You have approval from me starting from 2 days ago.
LGTM!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#discussion_r767192195
##########
File path: include/stdlib.h
##########
@@ -265,6 +265,8 @@ FAR void *bsearch(FAR const void *key, FAR const void *base, size_t nel,
size_t width, CODE int (*compar)(FAR const void *,
FAR const void *));
+FAR const char *getprogname(void);
Review comment:
Maybe add some comment above? Maybe something like `/* Current program name manipulation */`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-991891538
> > > In general current approach seems to be fine, but most probably adding `setprogname` will require adding additional field into `struct task_info_s`.
> >
> >
> > setprogname is normally used by glibc internally, to initialize the program name. But the program name is initialized automatically in NuttX, so setprogname isn't really needed here.
> > > BWT, maybe you can also fix a small bug in `gettextdomain` implementation by changing `#ifdef CONFIG_BUILD_KERNEL` to `#ifndef CONFIG_BUILD_KERNEL`. I just found it today during the code review.
> >
> >
> > Fixed here: #4982
>
> So we do not need neither `DEBUGASSERT(info != NULL);`, nor `return info->argv && info->argv[0] ? info->argv[0] : "?";` and assume that task info always exists and `argv` can't be `NULL` and `argv[0]` is not empty?
>
DEBUGASSERT could be used here to state the invariant explicitly, but the runtime check isn't needed to save the cost.
> If yes, then probably need to remove `task_get_info()` null checks in other files.
Whether to utilize assert is a personal preference, either is fine I think.
> I mean if we do not need any checks then maybe we can go with simpler `return task_get_info()->argv[0];`?
Yes, could be. But, the local variable make the code a little bit more readable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#discussion_r767190112
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
Review comment:
why do we need to include `stdlib.h` here?
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: getprogname
+ ****************************************************************************/
+
+FAR const char *getprogname(void)
+{
+ FAR struct task_info_s *info;
+
+ info = task_get_info();
+ return info->argv[0];
Review comment:
```suggestion
#ifndef CONFIG_BUILD_KERNEL
FAR struct task_info_s *info;
info = task_get_info();
DEBUGASSERT(info != NULL);
return (info->argv && info->argv[0]) ? info->argv[0] : "?";
#else
return "?";
#endif
```
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
Review comment:
```suggestion
#include <nuttx/config.h>
#include <assert.h>
#include <nuttx/tls.h>
```
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: getprogname
+ ****************************************************************************/
+
+FAR const char *getprogname(void)
+{
+ FAR struct task_info_s *info;
+
+ info = task_get_info();
+ return info->argv[0];
Review comment:
```suggestion
#ifndef CONFIG_BUILD_KERNEL
FAR struct task_info_s *info;
info = task_get_info();
DEBUGASSERT(info != NULL);
return info->argv && info->argv[0] ? info->argv[0] : "?";
#else
return "?";
#endif
```
##########
File path: include/stdlib.h
##########
@@ -265,6 +265,8 @@ FAR void *bsearch(FAR const void *key, FAR const void *base, size_t nel,
size_t width, CODE int (*compar)(FAR const void *,
FAR const void *));
+FAR const char *getprogname(void);
Review comment:
Maybe add some comment above? Maybe something like `/* Current program name manipulation */`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#discussion_r767190051
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: getprogname
+ ****************************************************************************/
+
+FAR const char *getprogname(void)
+{
+ FAR struct task_info_s *info;
+
+ info = task_get_info();
+ return info->argv[0];
Review comment:
```suggestion
#ifndef CONFIG_BUILD_KERNEL
FAR struct task_info_s *info;
info = task_get_info();
DEBUGASSERT(info != NULL);
return info->argv && info->argv[0] ? info->argv[0] : "?";
#else
return "?";
#endif
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-991874354
I mean if we do not need any checks then maybe we can go with simpler `return task_get_info()->argv[0];`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko edited a comment on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko edited a comment on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-991841572
> > In general current approach seems to be fine, but most probably adding `setprogname` will require adding additional field into `struct task_info_s`.
> >
>
> setprogname is normally used by glibc internally, to initialize the program name. But the program name is initialized automatically in NuttX, so setprogname isn't really needed here.
>
> > BWT, maybe you can also fix a small bug in `gettextdomain` implementation by changing `#ifdef CONFIG_BUILD_KERNEL` to `#ifndef CONFIG_BUILD_KERNEL`. I just found it today during the code review.
>
> Fixed here: https://github.com/apache/incubator-nuttx/pull/4982
So we do not need neither `DEBUGASSERT(info != NULL);`, nor `return info->argv && info->argv[0] ? info->argv[0] : "?";` and assume that task info always exists and `argv` can't be `NULL` and `argv[0]` is not empty?
If yes, then probably need to remove `task_get_info()` null checks in other files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#discussion_r767218079
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: getprogname
+ ****************************************************************************/
+
+FAR const char *getprogname(void)
+{
+ FAR struct task_info_s *info;
+
+ info = task_get_info();
+ return info->argv[0];
Review comment:
We don't check CONFIG_BUILD_KERNEL here, since argv field always exist in all three mode(flat, protected and kernel).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#discussion_r767218079
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: getprogname
+ ****************************************************************************/
+
+FAR const char *getprogname(void)
+{
+ FAR struct task_info_s *info;
+
+ info = task_get_info();
+ return info->argv[0];
Review comment:
We don't need check CONFIG_BUILD_KERNEL here, since argv field always exist in all three mode(flat, protected and kernel).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#discussion_r767218314
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
Review comment:
Done.
##########
File path: include/stdlib.h
##########
@@ -265,6 +265,8 @@ FAR void *bsearch(FAR const void *key, FAR const void *base, size_t nel,
size_t width, CODE int (*compar)(FAR const void *,
FAR const void *));
+FAR const char *getprogname(void);
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-991897466
Ok. But what if `argv[0]` is an empty string? Shouldn't `"?"` be returned in such case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-991911892
> Ok. But what if `argv[0]` is an empty string? Shouldn't `"?"` be returned in such case?
argv[0] should always exist from the POSIX/C standard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#discussion_r767190112
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
Review comment:
why do we need to include `stdlib.h` here?
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
+#include <stdlib.h>
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: getprogname
+ ****************************************************************************/
+
+FAR const char *getprogname(void)
+{
+ FAR struct task_info_s *info;
+
+ info = task_get_info();
+ return info->argv[0];
Review comment:
```suggestion
#ifndef CONFIG_BUILD_KERNEL
FAR struct task_info_s *info;
info = task_get_info();
DEBUGASSERT(info != NULL);
return (info->argv && info->argv[0]) ? info->argv[0] : "?";
#else
return "?";
#endif
```
##########
File path: libs/libc/stdlib/lib_getprogname.c
##########
@@ -0,0 +1,42 @@
+/****************************************************************************
+ * libs/libc/stdlib/lib_getprogname.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/tls.h>
Review comment:
```suggestion
#include <nuttx/config.h>
#include <assert.h>
#include <nuttx/tls.h>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-991946406
> > Ok. But what if `argv[0]` is an empty string? Shouldn't `"?"` be returned in such case?
>
> argv[0] should always exist from the POSIX/C standard.
Yes, but I'm curious if NuttX task can have `argv[0]` as `""` (an empty string)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-991841572
> > In general current approach seems to be fine, but most probably adding `setprogname` will require adding additional field into `struct task_info_s`.
> >
>
> setprogname is normally used by glibc internally, to initialize the program name. But the program name is initialized automatically in NuttX, so setprogname isn't really needed here.
>
> > BWT, maybe you can also fix a small bug in `gettextdomain` implementation by changing `#ifdef CONFIG_BUILD_KERNEL` to `#ifndef CONFIG_BUILD_KERNEL`. I just found it today during the code review.
>
> Fixed here: https://github.com/apache/incubator-nuttx/pull/4982
So we do not need neither `DEBUGASSERT(info != NULL);`, nor `return info->argv && info->argv[0] ? info->argv[0] : "?";` and assume that task info always exists and `argv` can't be `NULL` and `argv[0]` is not empty?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-993138503
> I see that in case if task name is `NULL` the default `"<noname>"` is picked. https://www.gnu.org/software/gnulib/manual/html_node/progname-and-getprogname.html states that `In some rare situations, it cannot determine the name; then it returns "?" instead.`, so that will not happen with NuttX tasks.
In GNU, program name is initialized in glibc startup, so there is chance user may call getprogname before the initialization is done and then return "?" in this case. But, NuttX initialize tcb_s->argv inside kernel before the main thread get the chance to run, so this case never happen at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #4978: libc: Add getprogname function
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #4978:
URL: https://github.com/apache/incubator-nuttx/pull/4978#issuecomment-994883558
> @pkarashchenko do you think this patch ready for merge?
Yes. You have approval from me starting from 2 days ago.
LGTM!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org