[Buildroot] [PATCH] quagga: Do not use fork on noMMU platforms

[Buildroot] [PATCH] quagga: Do not use fork on noMMU platforms

[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 <yuvaraj.patil@wipro.com>
---
 arch/Config.in              |    2 +-
 package/quagga/quagga.patch |   28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 package/quagga/quagga.patch

diff --git a/arch/Config.in b/arch/Config.in
index 77fae7a..9cd85a5 100644
--- a/arch/Config.in
+++ b/arch/Config.in
@@ -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
diff --git a/package/quagga/quagga.patch b/package/quagga/quagga.patch
new file mode 100644
index 0000000..2949b2c
--- /dev/null
+++ b/package/quagga/quagga.patch
@@ -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 -------------------------------
-- 
1.7.9.5

[Buildroot] [PATCH] quagga: Do not use fork on noMMU platforms

[Thomas Petazzoni]

Dear Yuvaraj Patil,

On Fri, 18 Jul 2014 17:27:43 +0530, Yuvaraj Patil wrote:
> fork() is not available in noMMU platforms, hence use vfork instead
> 
> fixes:
> http://autobuild.buildroot.net/results/1c5/1c5fdfe3a0248b65efdea0594d8367ff907015d4//
> 
> Signed-off-by: Yuvaraj Patil <yuvaraj.patil@wipro.com>
> ---
>  arch/Config.in              |    2 +-
>  package/quagga/quagga.patch |   28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
>  create mode 100644 package/quagga/quagga.patch
> 
> diff --git a/arch/Config.in b/arch/Config.in
> index 77fae7a..9cd85a5 100644
> --- a/arch/Config.in
> +++ b/arch/Config.in
> @@ -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.

> diff --git a/package/quagga/quagga.patch b/package/quagga/quagga.patch

Patches for packages should be named:

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

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

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

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

> @@ -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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] quagga: Do not use fork on noMMU platforms

[yuvaraj.patil at wipro.com]

Dear Thomas,



Thank you very much for your comments.



On Friday, July 18, 2014 5:34 PM, Thomas Petazzoni wrote:

>Patches for packages should be named:

>

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



I will follow this.



> 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.





>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,



On Fri, 18 Jul 2014 17:27:43 +0530, Yuvaraj Patil wrote:

> 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:yuvaraj.patil@wipro.com>>

> ---

>  arch/Config.in              |    2 +-

>  package/quagga/quagga.patch |   28 ++++++++++++++++++++++++++++

>  2 files changed, 29 insertions(+), 1 deletion(-)  create mode 100644

> package/quagga/quagga.patch

>

> diff --git a/arch/Config.in b/arch/Config.in index 77fae7a..9cd85a5

> 100644

> --- a/arch/Config.in

> +++ b/arch/Config.in

> @@ -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.



> diff --git a/package/quagga/quagga.patch b/package/quagga/quagga.patch



Patches for packages should be named:



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



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



> new file mode 100644

> index 0000000..2949b2c

> --- /dev/null

> +++ b/package/quagga/quagga.patch



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



> @@ -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

--

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>

[Buildroot] [PATCH] quagga: Do not use fork on noMMU platforms

[Thomas Petazzoni]

Hello,

On Sat, 19 Jul 2014 17:12:19 +0000, yuvaraj.patil at wipro.com wrote:

> > 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.

> 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.

> 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 <some.one@gmail.com>
---
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).

> >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?

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] quagga: Do not use fork on noMMU platforms

[yuvaraj.patil at wipro.com]

Hello Thomas,

On Sunday, July 20, 2014 3:40 PM, thomas.petazzoni at free-electrons.com wrote:

>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