[PATCH-next] modpost: Remove logically dead condition

[PATCH-next] modpost: Remove logically dead condition

[Advait Dhamorikar]

In case of failure vsnprintf returns `pos`, an unsigned long integer.
An unsigned value can never be negative, so this test will always evaluate 
the same way. 

Signed-off-by: Advait Dhamorikar <advaitdhamorikar@gmail.com>
---
 scripts/mod/file2alias.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 81f20ef13a0d..8ce48d7dd36d 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -57,11 +57,6 @@ module_alias_printf(struct module *mod, bool append_wildcard,
 	n = vsnprintf(NULL, 0, fmt, ap);
 	va_end(ap);
 
-	if (n < 0) {
-		error("vsnprintf failed\n");
-		return;
-	}
-
 	len = n + 1;	/* extra byte for '\0' */
 
 	if (append_wildcard)
-- 
2.34.1

Re: [PATCH-next] modpost: Remove logically dead condition

[Nicolas Schier]

On Wed, Nov 27, 2024 at 09:59:04PM +0530 Advait Dhamorikar wrote:
> In case of failure vsnprintf returns `pos`, an unsigned long integer.
> An unsigned value can never be negative, so this test will always evaluate 
> the same way. 

'man vsnprintf' on my system reveals a different behaviour:

| The  functions  snprintf() and vsnprintf() do not
| write more than size bytes (including the  termi‐
| nating  null  byte  ('\0')).   If  the output was
| truncated due to  this  limit,  then  the  return
| value  is the number of characters (excluding the
| terminating null  byte)  which  would  have  been
| written  to  the final string if enough space had
| been available.  Thus, a return value of size  or
| more  means  that the output was truncated.  (See
| also below under NOTES.)
|
| If an output error  is  encountered,  a  negative
| value is returned.

vsnprintf prototypes also indicate 'int' as return type.  What is the source of your mentioned findings?

Kind regards,
Nicolas

Re: [PATCH-next] modpost: Remove logically dead condition

[Advait Dhamorikar]

Hello Nicolas,

> vsnprintf prototypes also indicate 'int' as return type.  What is the source of your mentioned findings?
Sorry, I read an alternate vsnprintf implementation and have worded my
patch log wrong based on it.

However there is still an issue that n is declared as size_t which is
a typedef for
an unsigned long, I think the correct solution then is to use a signed
data type here for n?

Thanks for your time and feedback.

Best regards,
Advait

On Thu, 28 Nov 2024 at 02:21, Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Wed, Nov 27, 2024 at 09:59:04PM +0530 Advait Dhamorikar wrote:
> > In case of failure vsnprintf returns `pos`, an unsigned long integer.
> > An unsigned value can never be negative, so this test will always evaluate
> > the same way.
>
> 'man vsnprintf' on my system reveals a different behaviour:
>
> | The  functions  snprintf() and vsnprintf() do not
> | write more than size bytes (including the  termi‐
> | nating  null  byte  ('\0')).   If  the output was
> | truncated due to  this  limit,  then  the  return
> | value  is the number of characters (excluding the
> | terminating null  byte)  which  would  have  been
> | written  to  the final string if enough space had
> | been available.  Thus, a return value of size  or
> | more  means  that the output was truncated.  (See
> | also below under NOTES.)
> |
> | If an output error  is  encountered,  a  negative
> | value is returned.
>
> vsnprintf prototypes also indicate 'int' as return type.  What is the source of your mentioned findings?
>
> Kind regards,
> Nicolas
>

Re: [PATCH-next] modpost: Remove logically dead condition

[Masahiro Yamada]

On Thu, Nov 28, 2024 at 6:31 AM Advait Dhamorikar
<advaitdhamorikar@gmail.com> wrote:
>
> Hello Nicolas,
>
> > vsnprintf prototypes also indicate 'int' as return type.  What is the source of your mentioned findings?
> Sorry, I read an alternate vsnprintf implementation and have worded my
> patch log wrong based on it.
>
> However there is still an issue that n is declared as size_t which is
> a typedef for
> an unsigned long, I think the correct solution then is to use a signed
> data type here for n?


Yes.

'n' should be int.
This matches the return type of vsnprintf().


I will squash the following.



diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 81f20ef13a0d..5b5745f00eb3 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -49,7 +49,8 @@ module_alias_printf(struct module *mod, bool append_wildcard,
                    const char *fmt, ...)
 {
        struct module_alias *new, *als;
-       size_t len, n;
+       size_t len;
+       int n;
        va_list ap;

        /* Determine required size. */



Thank you for your report!




>
> Thanks for your time and feedback.
>
> Best regards,
> Advait
>
> On Thu, 28 Nov 2024 at 02:21, Nicolas Schier <nicolas@fjasle.eu> wrote:
> >
> > On Wed, Nov 27, 2024 at 09:59:04PM +0530 Advait Dhamorikar wrote:
> > > In case of failure vsnprintf returns `pos`, an unsigned long integer.
> > > An unsigned value can never be negative, so this test will always evaluate
> > > the same way.
> >
> > 'man vsnprintf' on my system reveals a different behaviour:
> >
> > | The  functions  snprintf() and vsnprintf() do not
> > | write more than size bytes (including the  termi‐
> > | nating  null  byte  ('\0')).   If  the output was
> > | truncated due to  this  limit,  then  the  return
> > | value  is the number of characters (excluding the
> > | terminating null  byte)  which  would  have  been
> > | written  to  the final string if enough space had
> > | been available.  Thus, a return value of size  or
> > | more  means  that the output was truncated.  (See
> > | also below under NOTES.)
> > |
> > | If an output error  is  encountered,  a  negative
> > | value is returned.
> >
> > vsnprintf prototypes also indicate 'int' as return type.  What is the source of your mentioned findings?
> >
> > Kind regards,
> > Nicolas
> >



-- 
Best Regards
Masahiro Yamada