quagga: Do not use fork on noMMU platforms

PATCH New Yuvaraj Patil 2 participants 5 messages Jul 18, 2014 – Jul 23, 2014

lore

Share & tools

lore

Yuvaraj Patil

author

2014-07-18 11:57 UTC #0 lore raw

PATCH 1/1 quagga: Do not use fork on noMMU platforms

fork() is not available in noMMU platforms, hence use vfork instead

fixes:
http://autobuild.buildroot.net/results/1c5/1c5fdfe3a0248b65efdea0594d8367ff907015d4//

Signed-off-by Yuvaraj Patil <[email protected]>

---
 arch/Config.in              |    2 +-
 package/quagga/quagga.patch |   28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 package/quagga/quagga.patch

arch/Config.in +1 -1

index 77fae7a..9cd85a5 100644
@@ -60,7 +60,7 @@ config BR2_avr32
 	# do the lack of upstream support in the major toolchain
 	# components. If you're interested by AVR32, contact the
 	# Buildroot community. Otherwise, its support will be removed
-	# in 2014.11.
+	# by the 2015.02 release.
 	depends on BR2_DEPRECATED_SINCE_2014_08
 	help
 	  The AVR32 is a 32-bit RISC microprocessor architecture designed by

package/quagga/quagga.patch new file +28 -0

new file mode 100644
index 0000000..2949b2c
@@ -0,0 +1,28 @@
+diff -Nurp quagga-0.99.23_orig/babeld/util.c quagga-0.99.23/babeld/util.c
+--- quagga-0.99.23_orig/babeld/util.c	2014-07-18 15:31:02.078625470 +0530
++++ quagga-0.99.23/babeld/util.c	2014-07-18 15:33:45.838618612 +0530
+@@ -430,7 +430,11 @@ daemonise()
+     fflush(stdout);
+     fflush(stderr);
+ 
++#ifdef HAVE_FORK
+     rc = fork();
++#else
++    rc = vfork();
++#endif
+     if(rc < 0)
+         return -1;
+ 
+diff -Nurp quagga-0.99.23_orig/configure.ac quagga-0.99.23/configure.ac
+--- quagga-0.99.23_orig/configure.ac	2014-07-18 15:31:02.098625470 +0530
++++ quagga-0.99.23/configure.ac	2014-07-18 15:32:41.710621299 +0530
+@@ -724,6 +724,9 @@ AC_CHECK_LIB(pam, pam_start,
+ fi
+ AC_SUBST(LIBPAM)
+ 
++dnl checks for library functions
++AC_CHECK_FUNC([fork])
++
+ dnl -------------------------------
+ dnl Endian-ness check
+ dnl -------------------------------

Signature · 1 line

1.7.9.5

Thomas Petazzoni

2014-07-18 12:03 UTC #1 lore raw

PATCH 1/1 quagga: Do not use fork on noMMU platforms

Dear Yuvaraj Patil,

Quoted 17 lines · Yuvaraj Patil

fork() is not available in noMMU platforms, hence use vfork instead
fixes:
http://autobuild.buildroot.net/results/1c5/1c5fdfe3a0248b65efdea0594d8367ff907015d4//
Signed-off-by Yuvaraj Patil <[email protected]>
---
 arch/Config.in              |    2 +-
 package/quagga/quagga.patch |   28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 package/quagga/quagga.patch
arch/Config.in +0 -0
index 77fae7a..9cd85a5 100644

arch/Config.in config BR2_avr32 · line 66

Quoted 8 lines

arch/Config.in +1 -1

@@ -60,7 +60,7 @@ config BR2_avr32
 	# do the lack of upstream support in the major toolchain
 	# components. If you're interested by AVR32, contact the
 	# Buildroot community. Otherwise, its support will be removed
-	# in 2014.11.
+	# by the 2015.02 release.
 	depends on BR2_DEPRECATED_SINCE_2014_08
 	help
 	  The AVR32 is a 32-bit RISC microprocessor architecture designed by

This chunk is not related to quagga, so it shouldn't be part of your patch.

package/quagga/quagga.patch

Quoted 1 line

package/quagga/quagga.patch +0 -0

Patches for packages should be named:

	<package>-<number>-<description>.patch.

See http://buildroot.org/downloads/manual/manual.html#patch-policy.

package/quagga/quagga.patch

Quoted 4 lines

new file mode 100644
index 0000000..2949b2c
package/quagga/quagga.patch new file +0 -0

Patches must have a description + Signed-off-by line.

package/quagga/quagga.patch line 12

Quoted 13 lines

package/quagga/quagga.patch new file +12 -0

@@ -0,0 +1,28 @@
+diff -Nurp quagga-0.99.23_orig/babeld/util.c quagga-0.99.23/babeld/util.c
+--- quagga-0.99.23_orig/babeld/util.c	2014-07-18 15:31:02.078625470 +0530
++++ quagga-0.99.23/babeld/util.c	2014-07-18 15:33:45.838618612 +0530
+@@ -430,7 +430,11 @@ daemonise()
+     fflush(stdout);
+     fflush(stderr);
+ 
++#ifdef HAVE_FORK
+     rc = fork();
++#else
++    rc = vfork();
++#endif

We have already received numerous patches like this from ADI, and each time our answer was the same: please include in the patch description a justification of why the fork() call can be replaced by vfork(). I'm sure you know that vfork() is not equivalent to fork() and that we cannot simply replace one by the other without checking carefully how fork() is used. With vfork(), the parent process is blocked until the child exits or calls exec(), and all changes made by the child will be visible by the parent. There are therefore many situations in which vfork() cannot be used as a replacement for fork().

We therefore want to see a justification for each patch making this replacement.

Also, notice that since commit
http://git.buildroot.net/buildroot/commit/?id=26b482990870bdc9921ceaa3204eaf513e0be165,
quagga can no longer be selected on Blackfin, as I've added a MMU dependency on the package. Of course, if you're interested in seeing quagga becoming available on Blackfin, some changes are needed. If you don't care specifically about quagga on Blackfin, we can live with it being non-available in Buildroot for Blackfin.

Best regards,

Thomas Petazzoni

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

yuvaraj.patil at wipro.com

author

2014-07-19 17:12 UTC #2 lore raw

PATCH 1/1 quagga: Do not use fork on noMMU platforms

Dear Thomas,

Thank you very much for your comments.

Quoted 5 lines · Thomas Petazzoni

Patches for packages should be named:
            <package>-<number>-<description>.patch.

I will follow this.

Quoted 1 line

Patches must have a description + Signed-off-by line.

I could see, Signed-off line is added in patch.

I used below command sequence (blue colour) to generate a patch.

#Diff to make a code patch

diff -Nurp <package_orig> <package> > <patch_name>.patch

#Move the diff generated patch to buildroot package directory

git add <patch name>.patch

git commit -a -s

#add the comment

git format-patch -1

#edit the patch to add comments

git send-email

Am I missing anything here? Please let me know so that I can take care for the next patch.

Quoted 5 lines

We have already received numerous patches like this from ADI, and each time our answer was the same: please include in the patch description a justification of why the fork() call can be replaced by vfork(). I'm sure >you know that vfork() is not equivalent to fork() and that we cannot simply replace one by the other without checking carefully how
fork() is used. With vfork(), the parent process is blocked until the child exits or calls exec(), and all changes made by the child will be visible by the parent. There are therefore many situations in which
vfork() cannot be used as a replacement for fork().

Can you please provide us a reference example (or a sample patch) from other architectures, how the fork() issue is fixed?

Similar issue is reported for few other packages on Blackfin and a reference example will help me to fix this issue in a correct way.

Thanks

Yuvaraj Patil

-----Original Message-----
From: Thomas Petazzoni [mailto:thomas.petazzoni at free-electrons.com]
Sent: Friday, July 18, 2014 5:34 PM
To: Yuvaraj Patil (WT01 - MFG & HITECH-HP-BANGALORE)
Cc: buildroot at busybox.net; adi-buildroot-devel at lists.sourceforge.net
Subject: Re: [PATCH] quagga: Do not use fork on noMMU platforms

Dear Yuvaraj Patil,

Quoted 41 lines · Yuvaraj Patil

fork() is not available in noMMU platforms, hence use vfork instead
fixes:
http://autobuild.buildroot.net/results/1c5/1c5fdfe3a0248b65efdea0594d8
367ff907015d4//
Signed-off-by Yuvaraj Patil <yuvaraj.patil at wipro.com<mailto:[email protected]>>
---
 arch/Config.in              |    2 +-
 package/quagga/quagga.patch |   28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)  create mode 100644
package/quagga/quagga.patch
arch/Config.in index 77fae7a..9cd85a5 renamed from arch/Config.in +0 -0
100644
--- a/arch/Config.in
+++ b/arch/Config.in
arch/Config.in index 77fae7a..9cd85a5 renamed from arch/Config.in +0 -0
@@ -60,7 +60,7 @@ config BR2_avr32
 
            # do the lack of upstream support in the major toolchain
 
            # components. If you're interested by AVR32, contact the
 
            # Buildroot community. Otherwise, its support will be removed

arch/Config.in index 77fae7a..9cd85a5 config BR2_avr32

Quoted 10 lines

arch/Config.in index 77fae7a..9cd85a5 renamed from arch/Config.in +1 -1

 
-           # in 2014.11.
 
+          # by the 2015.02 release.
 
            depends on BR2_DEPRECATED_SINCE_2014_08
 
            help
 
              The AVR32 is a 32-bit RISC microprocessor architecture designed by

This chunk is not related to quagga, so it shouldn't be part of your patch.

package/quagga/quagga.patch

Quoted 1 line

package/quagga/quagga.patch +0 -0

Patches for packages should be named:

                <package>-<number>-<description>.patch.

See http://buildroot.org/downloads/manual/manual.html#patch-policy.

Quoted 7 lines

new file mode 100644
index 0000000..2949b2c
--- /dev/null
+++ b/package/quagga/quagga.patch

Patches must have a description + Signed-off-by line.

Quoted 15 lines

package/quagga/quagga.patch +7 -0

@@ -0,0 +1,28 @@
 
+diff -Nurp quagga-0.99.23_orig/babeld/util.c quagga-0.99.23/babeld/util.c
 
+--- quagga-0.99.23_orig/babeld/util.c               2014-07-18 15:31:02.078625470 +0530
 
++++ quagga-0.99.23/babeld/util.c      2014-07-18 15:33:45.838618612 +0530
 
+@@ -430,7 +430,11 @@ daemonise()
 
+     fflush(stdout);
 
+     fflush(stderr);
 
+

package/quagga/quagga.patch line 12

Quoted 10 lines

package/quagga/quagga.patch +5 -0

 
++#ifdef HAVE_FORK
 
+     rc = fork();
 
++#else
 
++    rc = vfork();
 
++#endif

fork() is used. With vfork(), the parent process is blocked until the child exits or calls exec(), and all changes made by the child will be visible by the parent. There are therefore many situations in which

vfork() cannot be used as a replacement for fork().

We therefore want to see a justification for each patch making this replacement.

Also, notice that since commit

http://git.buildroot.net/buildroot/commit/?id=26b482990870bdc9921ceaa3204eaf513e0be165,

quagga can no longer be selected on Blackfin, as I've added a MMU dependency on the package. Of course, if you're interested in seeing quagga becoming available on Blackfin, some changes are needed. If you don't care specifically about quagga on Blackfin, we can live with it being non-available in Buildroot for Blackfin.

Best regards,

Thomas Petazzoni

Thomas Petazzoni, CTO, Free Electrons

Embedded Linux, Kernel and Android engineering http://free-electrons.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20140719/ed0ef6ab/attachment.html>

Thomas Petazzoni

2014-07-20 10:10 UTC #3 lore raw

PATCH 1/1 quagga: Do not use fork on noMMU platforms

Hello,

Quoted 3 lines · yuvaraj.patil at wipro.com

Quoted 1 line
Patches must have a description + Signed-off-by line.
I could see, Signed-off line is added in patch.

You're being confused because you're doing a patch that adds a patch, and we do require a Signed-off-by line on both.

We need a Signed-off-by in the commit log of the git commit that describes the Buildroot change, but we also require a Signed-off-by *inside* the patch you're adding, i.e inside the file that was named package/quagga/quagga.patch in your commit. We also require a description to be added in this file.

Quoted 9 lines

I used below command sequence (blue colour) to generate a patch.
#Diff to make a code patch
diff -Nurp <package_orig> <package> > <patch_name>.patch
#Move the diff generated patch to buildroot package directory

And here, you should edit this file, add a description and a Signed-off-by line.

Quoted 11 lines

git add <patch name>.patch
git commit -a -s
#add the comment
git format-patch -1
#edit the patch to add comments

Not necessarily needed. You can add whatever comments are needed while doing "git commit -a -s", as long as you format the commit log as follows:

======================================================================
<topic>: <description>

Blabla blabla description of the change.

Some more description of the change

Signed-off-by Some One <[email protected]>

---
And here some comments you want to send, but not have them included in the commit log.
======================================================================

Notice how "---" separates what git will include forever in its commit history (what's before the "---" sign) and what is only used to convey additional comments to the patch that should not be added to the commit permanently (what's after the "---" sign).

Quoted 12 lines

Quoted 11 lines
We have already received numerous patches like this from ADI, and each time our answer was the same: please include in the patch description a justification of why the fork() call can be replaced by vfork(). I'm sure >you know that vfork() is not equivalent to fork() and that we cannot simply replace one by the other without checking carefully how
fork() is used. With vfork(), the parent process is blocked until the child exits or calls exec(), and all changes made by the child will be visible by the parent. There are therefore many situations in which

Quoted 4 lines

Quoted 1 line
vfork() cannot be used as a replacement for fork().
Can you please provide us a reference example (or a sample patch) from other architectures, how the fork() issue is fixed?

Usually, we simply make the package not available on non-MMU architectures. So far, I don't think anybody submitted a patch turning fork() into vfork() with sufficient explanations for us to include the change.

Best regards,

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

yuvaraj.patil at wipro.com

author

2014-07-23 06:07 UTC #4 lore raw

PATCH 1/1 quagga: Do not use fork on noMMU platforms

Hello Thomas,

Quoted 4 lines · thomas.petazzoni at free-electrons.com

You're being confused because you're doing a patch that adds a patch, and we do require a Signed-off-by line on both.
We need a Signed-off-by in the commit log of the git commit that describes the Buildroot change, but we also require a Signed-off-by *inside* the patch you're adding, i.e inside the file that was named package/quagga/quagga.patch in your commit. We also require a description to be added in this file.

Thank you so much for the details information.
Your inputs helped me to generate a patch as per the expectation.
I have generated another patch for "gnuplot" package which I will be sending shortly. I hope this patch is as per the expectation.

Thanks
Yuvaraj patil