[PATCH v2] xfs: Replace strncpy with memcpy

[PATCH v2] xfs: Replace strncpy with memcpy

[Marcelo Moreira]

Following a suggestion from Dave and everyone who contributed to v1, this
changes modernizes the code by aligning it with current kernel best practices.
It improves code clarity and consistency, as strncpy is deprecated as explained
in Documentation/process/deprecated.rst. Furthermore, this change was tested
by xfstests and as it was not an easy task I decided to document on my blog
the step by step of how I did it https://meritissimo1.com/blog/2-xfs-tests :).

This change does not alter the functionality or introduce any behavioral
changes.

Changes include:
 - Replace strncpy with memcpy.

---
Changelog:

Changes since v1:
- Replace strncpy with memcpy instead of strscpy.
- The change was tested using xfstests.

Link to v1: https://lore.kernel.org/linux-kernel-mentees/CAPZ3m_jXwp1FfsvtR2s3nwATT3fER=Mc6qj+GzKuUhY5tjQFNQ@mail.gmail.com/T/#t

Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
---
 fs/xfs/scrub/symlink_repair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/scrub/symlink_repair.c b/fs/xfs/scrub/symlink_repair.c
index 953ce7be78dc..5902398185a8 100644
--- a/fs/xfs/scrub/symlink_repair.c
+++ b/fs/xfs/scrub/symlink_repair.c
@@ -185,7 +185,7 @@ xrep_symlink_salvage_inline(
 		return 0;
 
 	nr = min(XFS_SYMLINK_MAXLEN, xfs_inode_data_fork_size(ip));
-	strncpy(target_buf, ifp->if_data, nr);
+	memcpy(target_buf, ifp->if_data, nr);
 	return nr;
 }
 
-- 
2.50.1

Re: [PATCH v2] xfs: Replace strncpy with memcpy

[Christoph Hellwig]

On Sun, Aug 17, 2025 at 12:50:41PM -0300, Marcelo Moreira wrote:
> Following a suggestion from Dave and everyone who contributed to v1, this
> changes modernizes the code by aligning it with current kernel best practices.
> It improves code clarity and consistency, as strncpy is deprecated as explained
> in Documentation/process/deprecated.rst. Furthermore, this change was tested
> by xfstests and as it was not an easy task I decided to document on my blog
> the step by step of how I did it https://meritissimo1.com/blog/2-xfs-tests :).

I tried to follow the link, but got a warning about a potential security
threat because firefox doesn't trust the SSL certificate.  But maybe you
can add what you wrote up to the xfstests README or a new file linked
from it to help everyone using it first time?

Either way this probably shouldn't be in this kernel commit log.

The change itself looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] xfs: Replace strncpy with memcpy

[Christoph Hellwig]

On Sun, Aug 17, 2025 at 09:43:57PM -0700, Christoph Hellwig wrote:
> I tried to follow the link, but got a warning about a potential security
> threat because firefox doesn't trust the SSL certificate.

.. and if ignoring that redirects to a site that has the same issue.
I tried this for a few steps but didn't get anywhere even ignoring the
warnings as unlike what the warning claims you are very unlikely to
steal my credit card data by me rading your blog :)

Re: [PATCH v2] xfs: Replace strncpy with memcpy

[Marcelo Moreira]

Em seg., 18 de ago. de 2025 às 01:43, Christoph Hellwig
<hch@infradead.org> escreveu:
>
> On Sun, Aug 17, 2025 at 12:50:41PM -0300, Marcelo Moreira wrote:
> > Following a suggestion from Dave and everyone who contributed to v1, this
> > changes modernizes the code by aligning it with current kernel best practices.
> > It improves code clarity and consistency, as strncpy is deprecated as explained
> > in Documentation/process/deprecated.rst. Furthermore, this change was tested
> > by xfstests and as it was not an easy task I decided to document on my blog
> > the step by step of how I did it https://meritissimo1.com/blog/2-xfs-tests :).
>
> I tried to follow the link, but got a warning about a potential security
> threat because firefox doesn't trust the SSL certificate.  But maybe you
> can add what you wrote up to the xfstests README or a new file linked
> from it to help everyone using it first time?

Hmm, I've sent the same link to other people, and none of them
reported any issues with the SSL certificate. In my case, I'm using
Let's Encrypt. It could be an outdated certificate in your Firefox.
Regarding sending it to the official xfstests README, it would be a
good idea! If anyone here is involved in the xfstests project and
thinks the post is good, I can send it to the official README :D

> Either way this probably shouldn't be in this kernel commit log.
>
> The change itself looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks :)

-- 
Cheers,
Marcelo Moreira

Re: [PATCH v2] xfs: Replace strncpy with memcpy

[Christoph Hellwig]

On Wed, Aug 20, 2025 at 10:47:17AM -0300, Marcelo Moreira wrote:
> Hmm, I've sent the same link to other people, and none of them
> reported any issues with the SSL certificate. In my case, I'm using
> Let's Encrypt. It could be an outdated certificate in your Firefox.

Turns out I was connect to a corporate VPN which had some man in the
middle thing checking for "bad" websites, and they somehow considered
your bad, and their redirection didn't work.  Sorry for the blame :)

> Regarding sending it to the official xfstests README, it would be a
> good idea! If anyone here is involved in the xfstests project and
> thinks the post is good, I can send it to the official README :D

I think for the README itself it's probably a bit too specific.
But I'd love adding it as a new HOWTO file linked from README.

Re: [PATCH v2] xfs: Replace strncpy with memcpy

[Carlos Maiolino]

Hi.

On Sun, Aug 17, 2025 at 12:50:41PM -0300, Marcelo Moreira wrote:
> Following a suggestion from Dave and everyone who contributed to v1, this
> changes modernizes the code by aligning it with current kernel best practices.
> It improves code clarity and consistency, as strncpy is deprecated as explained
> in Documentation/process/deprecated.rst. Furthermore, this change was tested
> by xfstests

> and as it was not an easy task I decided to document on my blog
> the step by step of how I did it https://meritissimo1.com/blog/2-xfs-tests :).

The above line does not belong to the commit description. I'm glad
you've tested everything as we suggested and got to the point to run
xfstests which indeed is not a single-click button. But the patch
description is not a place to document it.

> 
> This change does not alter the functionality or introduce any behavioral
> changes.

^ This should be in the description...

> 
> Changes include:
>  - Replace strncpy with memcpy.

^ This is unnecessary. It's a plain copy/paste from the subject, no need
to write yet again.

> 
> ---

^ Keep a single --- in the patch... This is used as metadata, everything
  below the first --- is ignored by git.

> Changelog:
> 
> Changes since v1:
> - Replace strncpy with memcpy instead of strscpy.
> - The change was tested using xfstests.
> 
> Link to v1: https://lore.kernel.org/linux-kernel-mentees/CAPZ3m_jXwp1FfsvtR2s3nwATT3fER=Mc6qj+GzKuUhY5tjQFNQ@mail.gmail.com/T/#t
> 

^ All those Changelog metadata should be below the ---, so they don't
get into the commit message, but...

> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>

^ Those should be before the '---' otherwise, as I mentioned, git will
ignore those.

> ---
>  fs/xfs/scrub/symlink_repair.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/scrub/symlink_repair.c b/fs/xfs/scrub/symlink_repair.c
> index 953ce7be78dc..5902398185a8 100644
> --- a/fs/xfs/scrub/symlink_repair.c
> +++ b/fs/xfs/scrub/symlink_repair.c
> @@ -185,7 +185,7 @@ xrep_symlink_salvage_inline(
>  		return 0;
> 
>  	nr = min(XFS_SYMLINK_MAXLEN, xfs_inode_data_fork_size(ip));
> -	strncpy(target_buf, ifp->if_data, nr);
> +	memcpy(target_buf, ifp->if_data, nr);
>  	return nr;

The change looks fine. Once you fix the above points:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

Cheers,
Carlos

>  }
> 
> --
> 2.50.1
> 
> 

Re: [PATCH v2] xfs: Replace strncpy with memcpy

[Marcelo Moreira]

Em qui., 21 de ago. de 2025 às 06:39, Carlos Maiolino <cem@kernel.org> escreveu:
>
> Hi.

 Hi :)

> On Sun, Aug 17, 2025 at 12:50:41PM -0300, Marcelo Moreira wrote:
> > Following a suggestion from Dave and everyone who contributed to v1, this
> > changes modernizes the code by aligning it with current kernel best practices.
> > It improves code clarity and consistency, as strncpy is deprecated as explained
> > in Documentation/process/deprecated.rst. Furthermore, this change was tested
> > by xfstests
>
> > and as it was not an easy task I decided to document on my blog
> > the step by step of how I did it https://meritissimo1.com/blog/2-xfs-tests :).
>
> The above line does not belong to the commit description. I'm glad
> you've tested everything as we suggested and got to the point to run
> xfstests which indeed is not a single-click button. But the patch
> description is not a place to document it.
>
> >
> > This change does not alter the functionality or introduce any behavioral
> > changes.
>
> ^ This should be in the description...
>
> >
> > Changes include:
> >  - Replace strncpy with memcpy.
>
> ^ This is unnecessary. It's a plain copy/paste from the subject, no need
> to write yet again.
>
> >
> > ---
>
> ^ Keep a single --- in the patch... This is used as metadata, everything
>   below the first --- is ignored by git.
>
> > Changelog:
> >
> > Changes since v1:
> > - Replace strncpy with memcpy instead of strscpy.
> > - The change was tested using xfstests.
> >
> > Link to v1: https://lore.kernel.org/linux-kernel-mentees/CAPZ3m_jXwp1FfsvtR2s3nwATT3fER=Mc6qj+GzKuUhY5tjQFNQ@mail.gmail.com/T/#t
> >
>
> ^ All those Changelog metadata should be below the ---, so they don't
> get into the commit message, but...
>
> > Suggested-by: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
>
> ^ Those should be before the '---' otherwise, as I mentioned, git will
> ignore those.
>
> > ---
> >  fs/xfs/scrub/symlink_repair.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/scrub/symlink_repair.c b/fs/xfs/scrub/symlink_repair.c
> > index 953ce7be78dc..5902398185a8 100644
> > --- a/fs/xfs/scrub/symlink_repair.c
> > +++ b/fs/xfs/scrub/symlink_repair.c
> > @@ -185,7 +185,7 @@ xrep_symlink_salvage_inline(
> >               return 0;
> >
> >       nr = min(XFS_SYMLINK_MAXLEN, xfs_inode_data_fork_size(ip));
> > -     strncpy(target_buf, ifp->if_data, nr);
> > +     memcpy(target_buf, ifp->if_data, nr);
> >       return nr;
>
> The change looks fine. Once you fix the above points:

Ok, all points are clear to me, I'll send v3 soon. Thanks Carlos!

> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
:D

-- 
Cheers,
Marcelo Moreira

Re: [PATCH v2] xfs: Replace strncpy with memcpy

[Marcelo Moreira]

Em qui., 21 de ago. de 2025 às 05:47, Christoph Hellwig
<hch@infradead.org> escreveu:
>
> On Wed, Aug 20, 2025 at 10:47:17AM -0300, Marcelo Moreira wrote:
> > Hmm, I've sent the same link to other people, and none of them
> > reported any issues with the SSL certificate. In my case, I'm using
> > Let's Encrypt. It could be an outdated certificate in your Firefox.
>
> Turns out I was connect to a corporate VPN which had some man in the
> middle thing checking for "bad" websites, and they somehow considered
> your bad, and their redirection didn't work.  Sorry for the blame :)

Oh, ok haha, don't worry :)
>
> > Regarding sending it to the official xfstests README, it would be a
> > good idea! If anyone here is involved in the xfstests project and
> > thinks the post is good, I can send it to the official README :D
>
> I think for the README itself it's probably a bit too specific.
> But I'd love adding it as a new HOWTO file linked from README.

Hmm, cool, I can submit a patch to
git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git adding this new HOWTO
file :D
Thanks!

-- 
Cheers,
Marcelo Moreira