mm: mempolicy: the failure processing about mpol_to_str()

For the implementation (patch 1/3), need fill buffer as full as possible when buffer space is not enough.

For the caller (patch 2/3, 3/3), need check the return value of mpol_to_str().

Signed-off-by Chen Gang <[email protected]>

Quoted 7 lines · Chen Gang

For the implementation (patch 1/3), need fill buffer as full as possible when buffer space is not enough.

For the caller (patch 2/3, 3/3), need check the return value of mpol_to_str().

Signed-off-by Chen Gang <[email protected]>

Won't simple check for mpol_to_str() < 0 be enough? IOW fix all callers to check that mpol_to_str exit without errors. As far as I see here are only two users. Something like

show_numa_map

	ret = mpol_to_str();
	if (ret)
		return ret;

shmem_show_mpol

	ret = mpol_to_str();
	if (ret)
		return ret;

sure you'll have to change shmem_show_mpol statement to return int code. Won't this be more short and convenient?

need "if (ret < 0)" instead of. ;-)

Quoted 5 lines

sure you'll have to change shmem_show_mpol statement to return int code. Won't this be more short and convenient?

Hmm... if return -ENOSPC, in common processing, it still need continue (but need let outside know about the string truncation).

So I still suggest to give more check for it.

Quoted 10 lines · Chen Gang

Quoted 5 lines

sure you'll have to change shmem_show_mpol statement to return int code. Won't this be more short and convenient?

Hmm... if return -ENOSPC, in common processing, it still need continue (but need let outside know about the string truncation).

So I still suggest to give more check for it.

I still don't like adding additional code like

(quoted diff) +13 -0

+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+               switch (ret) {
+               case -ENOSPC:
+                       printk(KERN_WARNING
+                               "in %s: string is truncated in mpol_to_str().\n",
+                               __func__);
+               default:
+                       printk(KERN_ERR
+                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+                               __func__, ret, buffer, sizeof(buffer), mpol);
+                       return;
+               }

this code is pretty neat for debugging purpose I think but in most case (if only I've not missed something obvious) it simply won't be the case.

Won't somthing like below do the same but with smaller code change? Note I've not even compiled it but it shows the idea.

---
 fs/proc/task_mmu.c |    4 +++-
 mm/shmem.c         |   17 +++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

Index: linux-2.6.git/fs/proc/task_mmu.c
===================================================================

fs/proc/task_mmu.c +3 -1

@@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
 	walk.mm = mm;
 
 	pol = get_vma_policy(task, vma, vma->vm_start);
-	mpol_to_str(buffer, sizeof(buffer), pol);
+	n = mpol_to_str(buffer, sizeof(buffer), pol);
 	mpol_cond_put(pol);
+	if (n < 0)
+		return n;
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 

Index: linux-2.6.git/mm/shmem.c
===================================================================

mm/shmem.c +9 -8

@@ -883,16 +883,20 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;	/* show nothing */
 
-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+		return ret;
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)

@@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
 }
 #else /* !CONFIG_NUMA */
 #ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
-{
-}
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
 #endif /* CONFIG_TMPFS */
 
 static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,

@@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
 				from_kgid_munged(&init_user_ns, sbinfo->gid));
-	shmem_show_mpol(seq, sbinfo->mpol);
-	return 0;
+	return shmem_show_mpol(seq, sbinfo->mpol);
 }
 #endif /* CONFIG_TMPFS */
 

Quoted 14 lines · Cyrill Gorcunov

Quoted 10 lines · Chen Gang

Quoted 5 lines

sure you'll have to change shmem_show_mpol statement to return int code. Won't this be more short and convenient?

Hmm... if return -ENOSPC, in common processing, it still need continue (but need let outside know about the string truncation).

So I still suggest to give more check for it.

I still don't like adding additional code like

Review

Quoted 7 lines

(quoted diff) +7 -0

+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+               switch (ret) {
+               case -ENOSPC:
+                       printk(KERN_WARNING
+                               "in %s: string is truncated in mpol_to_str().\n",
+                               __func__);

Quoted 10 lines

(quoted diff) +6 -0

+               default:
+                       printk(KERN_ERR
+                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+                               __func__, ret, buffer, sizeof(buffer), mpol);
+                       return;
+               }

this code is pretty neat for debugging purpose I think but in most case (if only I've not missed something obvious) it simply won't be the case.

For mpol_to_str(), it is for printing string, I suggest to fill buffer as full as possible like another printing string functions, -ENOSPC is not critical error, callers may can bear it, and still want to continue.

For 2 callers, I still suggest to process '-ENOSPC' and continue, it is really not a critical error, they can continue.

For the 'default' error processing:

I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller. Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)

Thanks.

Quoted 74 lines

Won't somthing like below do the same but with smaller code change? Note I've not even compiled it but it shows the idea.

---
 fs/proc/task_mmu.c |    4 +++-
 mm/shmem.c         |   17 +++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

Index: linux-2.6.git/fs/proc/task_mmu.c
===================================================================

fs/proc/task_mmu.c +3 -1

@@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
 	walk.mm = mm;
 
 	pol = get_vma_policy(task, vma, vma->vm_start);
-	mpol_to_str(buffer, sizeof(buffer), pol);
+	n = mpol_to_str(buffer, sizeof(buffer), pol);
 	mpol_cond_put(pol);
+	if (n < 0)
+		return n;
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 

Index: linux-2.6.git/mm/shmem.c
===================================================================

mm/shmem.c +9 -8

@@ -883,16 +883,20 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;	/* show nothing */
 
-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+		return ret;
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)

@@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
 }
 #else /* !CONFIG_NUMA */
 #ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
-{
-}
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
 #endif /* CONFIG_TMPFS */
 
 static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,

@@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
 				from_kgid_munged(&init_user_ns, sbinfo->gid));
-	shmem_show_mpol(seq, sbinfo->mpol);
-	return 0;
+	return shmem_show_mpol(seq, sbinfo->mpol);
 }
 #endif /* CONFIG_TMPFS */
 

Quoted 15 lines · Chen Gang

Quoted 14 lines · Cyrill Gorcunov

Quoted 10 lines · Chen Gang

Quoted 5 lines

sure you'll have to change shmem_show_mpol statement to return int code. Won't this be more short and convenient?

Hmm... if return -ENOSPC, in common processing, it still need continue (but need let outside know about the string truncation).

So I still suggest to give more check for it.

I still don't like adding additional code like

Quoted 7 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+               switch (ret) {
+               case -ENOSPC:
+                       printk(KERN_WARNING
+                               "in %s: string is truncated in mpol_to_str().\n",
+                               __func__);

Oh, that need 'break' in my original patch. :-)

Quoted 26 lines

Quoted 10 lines

(quoted diff) +6 -0

+               default:
+                       printk(KERN_ERR
+                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+                               __func__, ret, buffer, sizeof(buffer), mpol);
+                       return;
+               }

this code is pretty neat for debugging purpose I think but in most case (if only I've not missed something obvious) it simply won't be the case.

For mpol_to_str(), it is for printing string, I suggest to fill buffer as full as possible like another printing string functions, -ENOSPC is not critical error, callers may can bear it, and still want to continue.

For 2 callers, I still suggest to process '-ENOSPC' and continue, it is really not a critical error, they can continue.

For the 'default' error processing:

I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller. Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)

Thanks.

Quoted 76 lines

Quoted 74 lines

Won't somthing like below do the same but with smaller code change? Note I've not even compiled it but it shows the idea.

---
 fs/proc/task_mmu.c |    4 +++-
 mm/shmem.c         |   17 +++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

Index: linux-2.6.git/fs/proc/task_mmu.c
===================================================================

fs/proc/task_mmu.c +3 -1

@@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
 	walk.mm = mm;
 
 	pol = get_vma_policy(task, vma, vma->vm_start);
-	mpol_to_str(buffer, sizeof(buffer), pol);
+	n = mpol_to_str(buffer, sizeof(buffer), pol);
 	mpol_cond_put(pol);
+	if (n < 0)
+		return n;
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 

Index: linux-2.6.git/mm/shmem.c
===================================================================

mm/shmem.c +9 -8

@@ -883,16 +883,20 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;	/* show nothing */
 
-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+		return ret;
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)

@@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
 }
 #else /* !CONFIG_NUMA */
 #ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
-{
-}
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
 #endif /* CONFIG_TMPFS */
 
 static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,

@@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
 				from_kgid_munged(&init_user_ns, sbinfo->gid));
-	shmem_show_mpol(seq, sbinfo->mpol);
-	return 0;
+	return shmem_show_mpol(seq, sbinfo->mpol);
 }
 #endif /* CONFIG_TMPFS */
 

Quoted 16 lines · Chen Gang

Quoted 15 lines · Chen Gang

Quoted 14 lines · Cyrill Gorcunov

Quoted 10 lines · Chen Gang

Quoted 5 lines

sure you'll have to change shmem_show_mpol statement to return int code. Won't this be more short and convenient?

Hmm... if return -ENOSPC, in common processing, it still need continue (but need let outside know about the string truncation).

So I still suggest to give more check for it.

I still don't like adding additional code like

Quoted 36 lines

Quoted 7 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+               switch (ret) {
+               case -ENOSPC:
+                       printk(KERN_WARNING
+                               "in %s: string is truncated in mpol_to_str().\n",
+                               __func__);

Oh, that need 'break' in my original patch. :-)

Quoted 26 lines

Quoted 10 lines

(quoted diff) +6 -0

+               default:
+                       printk(KERN_ERR
+                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+                               __func__, ret, buffer, sizeof(buffer), mpol);
+                       return;
+               }

this code is pretty neat for debugging purpose I think but in most case (if only I've not missed something obvious) it simply won't be the case.

For mpol_to_str(), it is for printing string, I suggest to fill buffer as full as possible like another printing string functions, -ENOSPC is not critical error, callers may can bear it, and still want to continue.

For 2 callers, I still suggest to process '-ENOSPC' and continue, it is really not a critical error, they can continue.

For the 'default' error processing:

I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller. Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)

Thanks.

Oh, for '-ENOSPC', it means critical error, it is my fault.

So, for simplify thinking and implementation, use your patch below is OK to me (but I suggest to print error information in the none return value function).

:-)

Quoted 61 lines

Quoted 61 lines

Quoted 61 lines

Won't somthing like below do the same but with smaller code change? Note I've not even compiled it but it shows the idea.

---
 fs/proc/task_mmu.c |    4 +++-
 mm/shmem.c         |   17 +++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

Index: linux-2.6.git/fs/proc/task_mmu.c
===================================================================

fs/proc/task_mmu.c +3 -1

@@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
 	walk.mm = mm;
 
 	pol = get_vma_policy(task, vma, vma->vm_start);
-	mpol_to_str(buffer, sizeof(buffer), pol);
+	n = mpol_to_str(buffer, sizeof(buffer), pol);
 	mpol_cond_put(pol);
+	if (n < 0)
+		return n;
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 

Index: linux-2.6.git/mm/shmem.c
===================================================================

mm/shmem.c +8 -6

@@ -883,16 +883,20 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;	/* show nothing */
 
-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+		return ret;
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)

@@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
 }
 #else /* !CONFIG_NUMA */
 #ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
-{
-}
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
 #endif /* CONFIG_TMPFS */
 

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

mm/shmem.c +0 -0

 static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,

@@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
 				from_kgid_munged(&init_user_ns, sbinfo->gid));

Quoted 8 lines

Quoted 8 lines

mm/shmem.c static int shmem_show_options(struct seq · line 2585

Quoted 8 lines

mm/shmem.c +1 -2

-	shmem_show_mpol(seq, sbinfo->mpol);
-	return 0;
+	return shmem_show_mpol(seq, sbinfo->mpol);
 }
 #endif /* CONFIG_TMPFS */
 

Quoted 4 lines

Quoted 2 lines

Quoted 123 lines · Chen Gang

Quoted 16 lines · Chen Gang

Quoted 15 lines · Chen Gang

Quoted 14 lines · Cyrill Gorcunov

Quoted 10 lines · Chen Gang

Quoted 5 lines

sure you'll have to change shmem_show_mpol statement to return int code. Won't this be more short and convenient?

Hmm... if return -ENOSPC, in common processing, it still need continue (but need let outside know about the string truncation).

So I still suggest to give more check for it.

I still don't like adding additional code like

Quoted 36 lines

Quoted 7 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+               switch (ret) {
+               case -ENOSPC:
+                       printk(KERN_WARNING
+                               "in %s: string is truncated in mpol_to_str().\n",
+                               __func__);

Oh, that need 'break' in my original patch. :-)

Quoted 26 lines

Quoted 10 lines

(quoted diff) +6 -0

+               default:
+                       printk(KERN_ERR
+                               "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+                               __func__, ret, buffer, sizeof(buffer), mpol);
+                       return;
+               }

this code is pretty neat for debugging purpose I think but in most case (if only I've not missed something obvious) it simply won't be the case.

For mpol_to_str(), it is for printing string, I suggest to fill buffer as full as possible like another printing string functions, -ENOSPC is not critical error, callers may can bear it, and still want to continue.

For 2 callers, I still suggest to process '-ENOSPC' and continue, it is really not a critical error, they can continue.

For the 'default' error processing:

I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller. Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)

Thanks.

Oh, for '-ENOSPC', it means critical error, it is my fault.

So, for simplify thinking and implementation, use your patch below is OK to me (but I suggest to print error information in the none return value function).

:-)

Quoted 61 lines

Quoted 61 lines

Quoted 61 lines

Won't somthing like below do the same but with smaller code change? Note I've not even compiled it but it shows the idea.

---
 fs/proc/task_mmu.c |    4 +++-
 mm/shmem.c         |   17 +++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

Index: linux-2.6.git/fs/proc/task_mmu.c
===================================================================

fs/proc/task_mmu.c +3 -1

@@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
 	walk.mm = mm;
 
 	pol = get_vma_policy(task, vma, vma->vm_start);
-	mpol_to_str(buffer, sizeof(buffer), pol);
+	n = mpol_to_str(buffer, sizeof(buffer), pol);
 	mpol_cond_put(pol);
+	if (n < 0)
+		return n;
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 

Index: linux-2.6.git/mm/shmem.c
===================================================================

mm/shmem.c +8 -6

@@ -883,16 +883,20 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;	/* show nothing */
 
-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+		return ret;
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)

@@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
 }
 #else /* !CONFIG_NUMA */
 #ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
-{
-}
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
 #endif /* CONFIG_TMPFS */
 

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

mm/shmem.c +0 -0

 static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,

@@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
 				from_kgid_munged(&init_user_ns, sbinfo->gid));

Quoted 8 lines

Quoted 8 lines

mm/shmem.c static int shmem_show_options(struct seq · line 2585

Quoted 8 lines

mm/shmem.c +1 -2

-	shmem_show_mpol(seq, sbinfo->mpol);
-	return 0;
+	return shmem_show_mpol(seq, sbinfo->mpol);
 }
 #endif /* CONFIG_TMPFS */
 

Oh, you have done, sorry.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

So, for simplify thinking and implementation, use your patch below is OK to me (but I suggest to print error information in the none return value function).

Quoted 3 lines · Chen Gang

So, for simplify thinking and implementation, use your patch below is OK to me (but I suggest to print error information in the none return value function).

Chen, I'm not going to dive into this area now, too busy with other stuff sorry, so if you consider my preliminary draft patch useful -- pick it up, modify it, test it and send to lkml then (just CC me).

Let shmem_show_mpol() return value, since it may fail.

mm/shmem.c +0 -0

@@ -883,16 +883,17 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS

Quoted 2 lines · On Wed, 21 Aug 2013 10:23:58 +0800 Chen Gang <[email protected]> wrote:

Let shmem_show_mpol() return value, since it may fail.

This patch has no effect.

Quoted 11 lines

mm/shmem.c +1 -1

@@ -883,16 +883,17 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
 

mm/shmem.c redirty: · line 893

Quoted 5 lines

mm/shmem.c +1 -1

 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;		/* show nothing */
 
 	mpol_to_str(buffer, sizeof(buffer), mpol);

Perhaps you meant to check the mpol_to_str() return value here.

mm/shmem.c redirty: · line 899

Quoted 5 lines

mm/shmem.c +1 -0

 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)

mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need check about it, or buffer may not be zero based, and next seq_printf() will cause issue.

Also need let shmem_show_mpol() return value, since it may fail.

Signed-off-by Chen Gang <[email protected]>

Reviewed-by Cyrill Gorcunov <[email protected]>

---
 mm/shmem.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

mm/shmem.c +10 -6

index f00c1c1..b4d44db 100644
@@ -883,16 +883,20 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;		/* show nothing */
 
-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+	if (ret < 0)
+		return ret;
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)

@@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
 }
 #else /* !CONFIG_NUMA */
 #ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
+	return 0;
 }
 #endif /* CONFIG_TMPFS */
 

@@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
 				from_kgid_munged(&init_user_ns, sbinfo->gid));
-	shmem_show_mpol(seq, sbinfo->mpol);
-	return 0;
+	return shmem_show_mpol(seq, sbinfo->mpol);
 }
 #endif /* CONFIG_TMPFS */
 

mm/shmem.c +1 -1

index f00c1c1..b4d44db 100644
@@ -883,16 +883,20 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)

mm/shmem.c +5 -1

+	if (ret < 0)
+		return ret;
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)

@@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
 }
 #else /* !CONFIG_NUMA */
 #ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
+	return 0;
 }
 #endif /* CONFIG_TMPFS */
 

Quoted 18 lines · Chen Gang

mm/shmem.c +3 -2

index f00c1c1..b4d44db 100644
@@ -883,16 +883,20 @@ redirty:
 
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
 	char buffer[64];
+	int ret;
 
 	if (!mpol || mpol->mode == MPOL_DEFAULT)
-		return;		/* show nothing */
+		return 0;		/* show nothing */
 

mm/shmem.c redirty: · line 894

Quoted 2 lines

mm/shmem.c +1 -1

-	mpol_to_str(buffer, sizeof(buffer), mpol);
+	ret = mpol_to_str(buffer, sizeof(buffer), mpol);

I was wondering how mpol_to_str() could fail given a pointer to a stack allocated buffer, so I checked and it happens if the mempolicy mode isn't known or the buffer isn't long enough.

I think it would be better to keep mpol_to_str() returning void, and hence avoiding the need for this patch, and make it so it cannot fail. If the mode is invalid, just store a 0 to the buffer (or "unknown"); and if maxlen isn't large enough, make it a compile-time error (let's avoid trying to be fancy and allocating less than 64 bytes on the stack if a given context is known to have short mempolicy strings).

Quoted 29 lines

mm/shmem.c +6 -3

+	if (ret < 0)
+		return ret;
 
 	seq_printf(seq, ",mpol=%s", buffer);
+	return 0;
 }
 
 static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)

@@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
 }
 #else /* !CONFIG_NUMA */
 #ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
+	return 0;
 }
 #endif /* CONFIG_TMPFS */
 

@@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
 	if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
 		seq_printf(seq, ",gid=%u",
 				from_kgid_munged(&init_user_ns, sbinfo->gid));
-	shmem_show_mpol(seq, sbinfo->mpol);
-	return 0;
+	return shmem_show_mpol(seq, sbinfo->mpol);
 }
 #endif /* CONFIG_TMPFS */
 

Quoted 7 lines

I think it would be better to keep mpol_to_str() returning void, and hence avoiding the need for this patch, and make it so it cannot fail. If the mode is invalid, just store a 0 to the buffer (or "unknown"); and if maxlen isn't large enough, make it a compile-time error (let's avoid trying to be fancy and allocating less than 64 bytes on the stack if a given context is known to have short mempolicy strings).

Hmm... at least, like most of print functions, it need return a value to tell the length it writes, so in my opinion, I still suggest it can return a value.

For common printing functions, caller knows about the string format and all parameters, and also can control them, so for callee, it is not 'quite polite' to return any failures to caller. :-)

But for our function, caller may not know about the string format and parameters' details, so callee has duty to check and process them:

e.g. "if related parameter is invalid, it is neccessary to notifiy to caller".

Quoted 8 lines · Chen Gang

Quoted 7 lines

I think it would be better to keep mpol_to_str() returning void, and hence avoiding the need for this patch, and make it so it cannot fail. If the mode is invalid, just store a 0 to the buffer (or "unknown"); and if maxlen isn't large enough, make it a compile-time error (let's avoid trying to be fancy and allocating less than 64 bytes on the stack if a given context is known to have short mempolicy strings).

Quoted 4 lines

Hmm... at least, like most of print functions, it need return a value to tell the length it writes, so in my opinion, I still suggest it can return a value.

Why? It can just store the string into the buffer pointed to by the char *buffer and terminate it appropriately while taking care that it doesn't exceed maxlen. Why does the caller need to know the number of bytes written? If it really does, you could just do strlen(buffer).

If there's a real reason for it, then that's fine, I just think it can be made to always succeed and never return < 0. (And why is nobody checking the return value today if it's so necessary?)

Quoted 7 lines

For common printing functions, caller knows about the string format and all parameters, and also can control them, so for callee, it is not 'quite polite' to return any failures to caller. :-)

But for our function, caller may not know about the string format and parameters' details, so callee has duty to check and process them:

Quoted 2 lines

e.g. "if related parameter is invalid, it is neccessary to notifiy to caller".

Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :) If the struct mempolicy really has a bad mode, then just store "unknown" or store a 0. If maxlen is insufficient for the longest possible string stored by mpol_to_str(), then it should be a compile-time error.

Quoted 9 lines

Why? It can just store the string into the buffer pointed to by the char *buffer and terminate it appropriately while taking care that it doesn't exceed maxlen. Why does the caller need to know the number of bytes written? If it really does, you could just do strlen(buffer).

If there's a real reason for it, then that's fine, I just think it can be made to always succeed and never return < 0. (And why is nobody checking the return value today if it's so necessary?)

For common printing functions: sprintf(), snprintf(), scnprintf().

For some of specific printing functions: drivers/usb/host/uhci-debug.c.

at least they can let caller easy use.

Quoted 6 lines

Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :) If the struct mempolicy really has a bad mode, then just store "unknown" or store a 0. If maxlen is insufficient for the longest possible string stored by mpol_to_str(), then it should be a compile-time error.

Hmm... what you said sounds reasonable if mpol_to_str() is a normal static funciton (only used within a file).

For extern function, callee (inside) can not assume anything of caller (outside) beyond the interface. So if failure occurs, better to report to caller only, and let caller to check what to do next.

Quoted 10 lines · Chen Gang

Quoted 9 lines

Why? It can just store the string into the buffer pointed to by the char *buffer and terminate it appropriately while taking care that it doesn't exceed maxlen. Why does the caller need to know the number of bytes written? If it really does, you could just do strlen(buffer).

If there's a real reason for it, then that's fine, I just think it can be made to always succeed and never return < 0. (And why is nobody checking the return value today if it's so necessary?)

Quoted 6 lines

For common printing functions: sprintf(), snprintf(), scnprintf().

For some of specific printing functions: drivers/usb/host/uhci-debug.c.

at least they can let caller easy use.

Nobody needs mpol_to_str() to return the number of characters written, period. It's one of the most trivial functions you're going to see in the mempolicy code, it takes a pointer to a buffer and it stores characters to it for display. Nobody is going to use it for anything else. Let's not overcomplicate this trivial function.

Quoted 7 lines

Quoted 6 lines

Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :) If the struct mempolicy really has a bad mode, then just store "unknown" or store a 0. If maxlen is insufficient for the longest possible string stored by mpol_to_str(), then it should be a compile-time error.

Quoted 7 lines

Hmm... what you said sounds reasonable if mpol_to_str() is a normal static funciton (only used within a file).

For extern function, callee (inside) can not assume anything of caller (outside) beyond the interface. So if failure occurs, better to report to caller only, and let caller to check what to do next.

Are you just preaching about the best practices of software engineering? mpol_to_str() should never fail at runtime, plain and simple. If somebody introduces a new mode and doesn't update it to print correctly, let's not fail the read(). Let's just print "unknown". And if someone passes too small of a buffer, break it at compile time so it gets noticed and fixed.

I guarantee you that any kernel developer who writes code to call mpol_to_str() will be happy it never fails at runtime. Really.

Quoted 54 lines · David Rientjes

Quoted 12 lines · Chen Gang

Quoted 11 lines

Why? It can just store the string into the buffer pointed to by the char *buffer and terminate it appropriately while taking care that it doesn't exceed maxlen. Why does the caller need to know the number of bytes written? If it really does, you could just do strlen(buffer).

If there's a real reason for it, then that's fine, I just think it can be
made to always succeed and never return < 0. (And why is nobody checking
the return value today if it's so necessary?)

Quoted 6 lines

For common printing functions: sprintf(), snprintf(), scnprintf().

For some of specific printing functions: drivers/usb/host/uhci-debug.c.

at least they can let caller easy use.

Nobody needs mpol_to_str() to return the number of characters written, period. It's one of the most trivial functions you're going to see in the
mempolicy code, it takes a pointer to a buffer and it stores characters to
it for display. Nobody is going to use it for anything else. Let's not overcomplicate this trivial function.

Quoted 10 lines

Quoted 9 lines

Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)
If the struct mempolicy really has a bad mode, then just store "unknown"
or store a 0. If maxlen is insufficient for the longest possible string
stored by mpol_to_str(), then it should be a compile-time error.

Quoted 7 lines

Hmm... what you said sounds reasonable if mpol_to_str() is a normal static funciton (only used within a file).

For extern function, callee (inside) can not assume anything of caller (outside) beyond the interface. So if failure occurs, better to report to caller only, and let caller to check what to do next.

Are you just preaching about the best practices of software engineering? mpol_to_str() should never fail at runtime, plain and simple. If somebody
introduces a new mode and doesn't update it to print correctly, let's not fail the read(). Let's just print "unknown". And if someone passes too small of a buffer, break it at compile time so it gets noticed and fixed.

Quoted 2 lines

I guarantee you that any kernel developer who writes code to call mpol_to_str() will be happy it never fails at runtime. Really.

Agreed. Even though we don't change mpol_to_str() interface, please just add BUG_ON into shmem_show_mpol(). It is much simpler than current proposal.

At least, currently mpol_to_str() already have following assertion. I mean, the code assume every developer know maximum length of mempolicy. I have no seen any reason to bring addional complication to shmem area.

    /*
     * Sanity check:  room for longest mode, flag and some nodes
     */
    VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);

Thanks.

At least, currently mpol_to_str() already have following assertion. I mean, the code assume every developer know maximum length of mempolicy. I have no seen any reason to bring addional complication to shmem area.

	/*
	 * Sanity check:  room for longest mode, flag and some nodes
	 */
	VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);

Quoted 10 lines · KOSAKI Motohiro

At least, currently mpol_to_str() already have following assertion. I mean, the code assume every developer know maximum length of mempolicy. I have no seen any reason to bring addional complication to shmem area.

	/*
	 * Sanity check:  room for longest mode, flag and some nodes
	 */
	VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);

No need to make it a runtime error, the value passed as maxlen is a constant, as is the use of sizeof(buffer), so the value is known at compile-time. You can make this a BUILD_BUG_ON() if you are creative.

Quoted 13 lines · David Rientjes

Quoted 10 lines · KOSAKI Motohiro

At least, currently mpol_to_str() already have following assertion. I mean, the code assume every developer know maximum length of mempolicy. I have no seen any reason to bring addional complication to shmem area.

	/*
	 * Sanity check:  room for longest mode, flag and some nodes
	 */
	VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);

Quoted 3 lines

No need to make it a runtime error, the value passed as maxlen is a constant, as is the use of sizeof(buffer), so the value is known at compile-time. You can make this a BUILD_BUG_ON() if you are creative.

Making compile time error brings us another complication. I'd like to keep just one line assertion.

Hmm... I am not quite sure: a C compiler is clever enough to know about that.

At least, for ANSI C definition, the C compiler has no duty to know about that.

And it is not for an optimization, either, so I guess the C compiler has no enought interests to support this features (know about that).

Quoted 3 lines · Chen Gang

Hmm... I am not quite sure: a C compiler is clever enough to know about that.

Quoted 6 lines

At least, for ANSI C definition, the C compiler has no duty to know about that.

And it is not for an optimization, either, so I guess the C compiler has no enought interests to support this features (know about that).

What on earth are we talking about in this thread?

Rename mpol_to_str() to __mpol_to_str(). Make a static inline function in mempolicy.h named mpol_to_str(). That function does BUILD_BUG_ON(maxlen <
64) and then calls __mpol_to_str().

Modify __mpol_to_str() to store "unknown" when mpol->mode does not match any known MPOL_* constant.

Both functions can now return void.

This is like a ten line diff. Seriously.

Quoted 7 lines

Rename mpol_to_str() to __mpol_to_str(). Make a static inline function in mempolicy.h named mpol_to_str(). That function does BUILD_BUG_ON(maxlen <
64) and then calls __mpol_to_str().

Modify __mpol_to_str() to store "unknown" when mpol->mode does not match any known MPOL_* constant.

Can we be sure 'maxlen' should not be less than 64? For show_numa_map() in fs/proc/task_mmu.c, it use 50 which is less than 64, is it correct?

Can we be sure that our output contents are always less than 64 bytes? Do we need BUG_ON() instead of all '-ENOSPC' in mpol_to_str()?

Hmm... If assume what you said above was always correct: "we are always sure 64 bytes is enough, and 'maxlen' should be never less than 64".

It would be better to use a structure (which has a member "char buf[64]") pointer instead of 'buffer' and 'maxlen'.
(and also still need check 64 memory bondary and '\0' within mpol_to_str).

Quoted 8 lines · Chen Gang

Quoted 7 lines

Rename mpol_to_str() to __mpol_to_str(). Make a static inline function in mempolicy.h named mpol_to_str(). That function does BUILD_BUG_ON(maxlen <
64) and then calls __mpol_to_str().

Modify __mpol_to_str() to store "unknown" when mpol->mode does not match any known MPOL_* constant.

Quoted 3 lines

Can we be sure 'maxlen' should not be less than 64? For show_numa_map() in fs/proc/task_mmu.c, it use 50 which is less than 64, is it correct?

Whatever the max string length is that can be stored by mpol_to_str() preferably rounded to the nearest power of two.

Quoted 3 lines

Can we be sure that our output contents are always less than 64 bytes? Do we need BUG_ON() instead of all '-ENOSPC' in mpol_to_str()?

You can determine the maximum string length by looking at the implementation of mpol_to_str().

Quoted 6 lines

Hmm... If assume what you said above was always correct: "we are always sure 64 bytes is enough, and 'maxlen' should be never less than 64".

It would be better to use a structure (which has a member "char buf[64]") pointer instead of 'buffer' and 'maxlen'.
(and also still need check 64 memory bondary and '\0' within mpol_to_str).

That's ridiculous, kernel developers who call mpol_to_str() aren't idiots.

I think at this point it will just be best if I propose a patch and ask for it to be merged into the -mm tree rather than continue this thread.

Quoted 14 lines · Chen Gang

Quoted 9 lines

Why? It can just store the string into the buffer pointed to by the char *buffer and terminate it appropriately while taking care that it doesn't exceed maxlen. Why does the caller need to know the number of bytes written? If it really does, you could just do strlen(buffer).

If there's a real reason for it, then that's fine, I just think it can be made to always succeed and never return < 0. (And why is nobody checking the return value today if it's so necessary?)

For common printing functions: sprintf(), snprintf(), scnprintf().

For some of specific printing functions: drivers/usb/host/uhci-debug.c.

Quoted 2 lines

at least they can let caller easy use.

Nobody needs mpol_to_str() to return the number of characters written, period. It's one of the most trivial functions you're going to see in the mempolicy code, it takes a pointer to a buffer and it stores characters to it for display. Nobody is going to use it for anything else. Let's not overcomplicate this trivial function.

Quoted 7 lines

Quoted 6 lines

Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :) If the struct mempolicy really has a bad mode, then just store "unknown" or store a 0. If maxlen is insufficient for the longest possible string stored by mpol_to_str(), then it should be a compile-time error.

Quoted 7 lines

Hmm... what you said sounds reasonable if mpol_to_str() is a normal static funciton (only used within a file).

For extern function, callee (inside) can not assume anything of caller (outside) beyond the interface. So if failure occurs, better to report to caller only, and let caller to check what to do next.

Are you just preaching about the best practices of software engineering? mpol_to_str() should never fail at runtime, plain and simple. If somebody introduces a new mode and doesn't update it to print correctly, let's not fail the read(). Let's just print "unknown". And if someone passes too small of a buffer, break it at compile time so it gets noticed and fixed.

I guarantee you that any kernel developer who writes code to call mpol_to_str() will be happy it never fails at runtime. Really.

BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be replaced by returning -EINVAL.

Quoted 2 lines

BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be replaced by returning -EINVAL.

Nope. mpol_to_str() is not carefully designed since it was born. It doesn't have a way to get proper buffer size. That said, the function assume all caller know proper buffer size. So, just adding EINVAL doesn't solve anything. we need to add a way to get proper buffer length at least if we take your way. However it is overengineering because current all caller doesn't need it.

---
  mm/shmem.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

mm/shmem.c +0 -0

index 8612a95..3f81120 100644

Quoted 13 lines

---
  mm/shmem.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

mm/shmem.c +0 -0

index 8612a95..3f81120 100644
@@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq,
 struct mempolicy *mpol)
      if (!mpol || mpol->mode == MPOL_DEFAULT)
          return;        /* show nothing */
 

mm/shmem.c static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) · line 893

Quoted 2 lines

mm/shmem.c +1 -1

-    mpol_to_str(buffer, sizeof(buffer), mpol);
+    VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);

NAK. VM_BUG_ON is a kind of assertion. It erase the contents if CONFIG_DEBUG_VM not set.
An argument of assertion should not have any side effect.

Quoted 13 lines · KOSAKI Motohiro

Quoted 13 lines

---
   mm/shmem.c |    2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)

mm/shmem.c +0 -0

index 8612a95..3f81120 100644
@@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq,
 struct mempolicy *mpol)
       if (!mpol || mpol->mode == MPOL_DEFAULT)
           return;        /* show nothing */
 

Quoted 6 lines

mm/shmem.c static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) · line 893

Quoted 2 lines

mm/shmem.c +1 -1

-    mpol_to_str(buffer, sizeof(buffer), mpol);
+    VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);

NAK. VM_BUG_ON is a kind of assertion. It erase the contents if CONFIG_DEBUG_VM not set.
An argument of assertion should not have any side effect.

Oh, really it is. In my opinion, need use "BUG_ON(mpol_to_str() < 0)" instead of "VM_BUG_ON(mpol_to_str() < 0);".

Quoted 21 lines · Chen Gang

Quoted 19 lines · KOSAKI Motohiro

Quoted 9 lines

---
   mm/shmem.c |    2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)

mm/shmem.c +0 -0

index 8612a95..3f81120 100644

mm/shmem.c static void shmem_show_mpol(struct seq_file *seq, · line 894

Quoted 6 lines

mm/shmem.c +1 -1

@@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq,
 struct mempolicy *mpol)
       if (!mpol || mpol->mode == MPOL_DEFAULT)
           return;        /* show nothing */
 
-    mpol_to_str(buffer, sizeof(buffer), mpol);
+    VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);

NAK. VM_BUG_ON is a kind of assertion. It erase the contents if CONFIG_DEBUG_VM not set.
An argument of assertion should not have any side effect.

Quoted 2 lines

Oh, really it is. In my opinion, need use "BUG_ON(mpol_to_str() < 0)" instead of "VM_BUG_ON(mpol_to_str() < 0);".

BUG_ON() is safe. but I still don't like it. As far as I heard, Google changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google. Please treat an assertion as assertion. Not any other something.

Quoted 4 lines

BUG_ON() is safe. but I still don't like it. As far as I heard, Google changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google. Please treat an assertion as assertion. Not any other something.

Hmm... in kernel wide, BUG_ON() is 'common' 'standard' assertion, and "mm/" is a common sub-system (not architecture specific), so when we use BUG_ON(), we already 'express' our 'opinion' enough to readers.

Quoted 4 lines · Chen Gang

Quoted 4 lines

BUG_ON() is safe. but I still don't like it. As far as I heard, Google changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google. Please treat an assertion as assertion. Not any other something.

Google does not disable BUG_ON(), sheesh.

Quoted 4 lines

Hmm... in kernel wide, BUG_ON() is 'common' 'standard' assertion, and "mm/" is a common sub-system (not architecture specific), so when we use BUG_ON(), we already 'express' our 'opinion' enough to readers.

That's ridiculous, we're not going to panic the kernel at runtime because a buffer is too small. Make it a compile-time error like I suggested so we catch this before we even build the kernel.

BUG_ON() is widely and commonly used in kernel wide, and BUG_ON() can be customized by any architectures, so I guess, if google really think it is necessary, it will customize it.

If "compile-time error" will make code complex to both readers and writers (e.g. our case), forcing "compile-time error" may still be good enough to google, but may not be good enough for others.

So in my opinion, for our case which is a common sub-system, not an architecture specific sub-system, better use "run-time error".

Hmm... for extern function, at present, maybe you can guarantee, but may not always in the future. And "Code is mainly for making readers 'happy' (e.g version mergers), not writers".

For extern function which more than 50 lines (used by 2 sub-systems), it is strange for readers to find it no return value, also strange to let *BUG_ON() on the extern function's input parameters directly.

If one caller wants to treat failure as BUG, can "*BUG_ON(mpol_to_str() < 0)", that will be more clearer to all members (need this patch do like it? :-) ).

BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be replaced by returning -EINVAL.

Quoted 13 lines · Chen Gang

Hmm... for extern function, at present, maybe you can guarantee, but may not always in the future. And "Code is mainly for making readers 'happy' (e.g version mergers), not writers".

For extern function which more than 50 lines (used by 2 sub-systems), it is strange for readers to find it no return value, also strange to let *BUG_ON() on the extern function's input parameters directly.

If one caller wants to treat failure as BUG, can "*BUG_ON(mpol_to_str() < 0)", that will be more clearer to all members (need this patch do like it? :-) ).

Quoted 3 lines

BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be replaced by returning -EINVAL.

Are you reading my emails?

I'm asking for a compile-time error if the maxlen passed to mpol_to_str() is too small; it's a constant value and can be evaluated at compile-time. Then mpol_to_str() can return void if you simply store "unknown" when it's an unknown mode.

Sheesh.

mm/mempolicy.c out: · line 2871

Quoted 5 lines

mm/mempolicy.c +0 -1

 	case MPOL_BIND:
-		/* Fall through */
 	case MPOL_INTERLEAVE:
 		nodes = pol->v.nodes;
 		break;

Any reason not to leave this ?

"missing break" is the 2nd most common thing that coverity picks up. Most of them are false positives like the above, but the lack of annotations in our source makes it time-consuming to pick through them all to find the real bugs.

Quoted 8 lines · Dave Jones

mm/mempolicy.c out: · line 2871

Quoted 5 lines

mm/mempolicy.c +0 -1

 	case MPOL_BIND:
-		/* Fall through */
 	case MPOL_INTERLEAVE:
 		nodes = pol->v.nodes;
 		break;

Any reason not to leave this ?

Quoted 5 lines

"missing break" is the 2nd most common thing that coverity picks up. Most of them are false positives like the above, but the lack of annotations in our source makes it time-consuming to pick through them all to find the real bugs.

Check out things like drivers/mfd/wm5110-tables.c that do things like

	switch (reg) {
	case ARIZONA_SOFTWARE_RESET:
	case ARIZONA_DEVICE_REVISION:
	case ARIZONA_CTRL_IF_SPI_CFG_1:
	case ARIZONA_CTRL_IF_I2C1_CFG_1:
	case ARIZONA_CTRL_IF_I2C2_CFG_1:
	case ARIZONA_CTRL_IF_I2C1_CFG_2:
	case ARIZONA_CTRL_IF_I2C2_CFG_2:
	...

and that file has over 1,000 case statements. Having a

	/* fall through */

for all of them would be pretty annoying.

I don't remember any coding style rule about this (in fact Documentation/CodingStyle has examples of case statements without such a comment), I think it's just personal preference so I'll leave it to Andrew and what he prefers.

(And if he prefers the /* fall through */ then we should ask that it be added to checkpatch.pl since it warns about a million other things and not this.)

Quoted 3 lines

	/* fall through */

for all of them would be pretty annoying.

agreed, but with that example, it seems pretty obvious (to me at least) that the lack of break's is intentional. Where it gets trickier to make quick judgment calls is cases like the one I mentioned above, where there are only a few cases, and there's real code involved in some but not all cases.

Quoted 10 lines · Dave Jones

Quoted 3 lines

	/* fall through */

for all of them would be pretty annoying.

agreed, but with that example, it seems pretty obvious (to me at least) that the lack of break's is intentional. Where it gets trickier to make quick judgment calls is cases like the one I mentioned above, where there are only a few cases, and there's real code involved in some but not all cases.

I fully agree and have code in the oom killer that has the "fall through" comment if there's code in between the case statements, but I think things like

	case MPOL_BIND:
	case MPOL_INTERLEAVE:
		...

is quite easy to read. I don't feel strongly at all, though, so I'll just leave it to Andrew's preference.

Quoted 10 lines

I fully agree and have code in the oom killer that has the "fall through" comment if there's code in between the case statements, but I think things like

	case MPOL_BIND:
	case MPOL_INTERLEAVE:
		...

is quite easy to read. I don't feel strongly at all, though, so I'll just leave it to Andrew's preference.

I've never even thought about it, but that won't prevent me from pretending otherwise! How about:

This:

	case WIBBLE:
		something();
		something_else();
	case WOBBLE:

needs a /* fall through */ comment (because it *looks* like a mistake), whereas

case WIBBLE:
case WOBBLE:

does not?

mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need check about it, or buffer may not be zero based, and next seq_printf() will cause issue.

Signed-off-by Chen Gang <[email protected]>

Cc Cyrill Gorcunov <[email protected]>

Quoted 4 lines · Chen Gang

mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need check about it, or buffer may not be zero based, and next seq_printf() will cause issue.

Quoted 2 lines

Signed-off-by Chen Gang <[email protected]>

Cc Cyrill Gorcunov <[email protected]>

Looks good to me, thanks!

Reviewed-by Cyrill Gorcunov <[email protected]>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>