Mailing List Archive

Hi Hideo-san,

Sorry for picking up this so late.

On Tue, Nov 29, 2011 at 02:48:52PM +0900, renayama19661014@ybb.ne.jp wrote:
> Hi All,
>
> We made a patch to IPsrcaddr.
>
> This patch revises the next point.
>
> * When route has been already assigned, RA skips an allotment.

Is this just a performance improvement? Or did you see anything
wrong happen when running the current code?

> * Added error log of FINDIF.
> * Deleted the unused sentence.

It would be good to have at least two patches, because we should
always try to have patches with single self-contained
modification.

Cheers,

Dejan

> Please please confirm my correction.
> And please commit a correction.
>
> Best Regards,
> Hideo Yamauchi

> diff -r 2107bc4f5c8b heartbeat/IPsrcaddr
> --- a/heartbeat/IPsrcaddr Thu Nov 24 14:13:11 2011 +0900
> +++ b/heartbeat/IPsrcaddr Thu Nov 24 14:13:53 2011 +0900
> @@ -167,13 +167,20 @@
> srca_start() {
> srca_read $1
>
> - ip route replace $NETWORK dev $INTERFACE src $1 || \
> - errorexit "command 'ip route replace $NETWORK dev $INTERFACE src $1' failed"
> + rc=$?
> + if [ $rc = 0 ]; then
> + rc=$OCF_SUCCESS
> + ocf_log info "The ip route has been already set.($NETWORK, $INTERFACE, $ROUTE_WO_SRC)"
> + else
> + ip route replace $NETWORK dev $INTERFACE src $1 || \
> + errorexit "command 'ip route replace $NETWORK dev $INTERFACE src $1' failed"
>
> - $CMDCHANGE $ROUTE_WO_SRC src $1 || \
> - errorexit "command '$CMDCHANGE $ROUTE_WO_SRC src $1' failed"
> + $CMDCHANGE $ROUTE_WO_SRC src $1 || \
> + errorexit "command '$CMDCHANGE $ROUTE_WO_SRC src $1' failed"
> + rc=$?
> + fi
>
> - return $?
> + return $rc
> }
>
> #
> @@ -252,7 +259,6 @@
> else
> true
> fi
> -# return $OCF_SUCCESS
> ;;
> *) #less than three decimal dots
> false;;
> @@ -377,7 +383,6 @@
>
> Linux|SunOS)
> IF=`find_interface "$BASEIP"`
> -# echo $IF
> if [ -z "$IF" ]; then
> return $OCF_NOT_RUNNING
> fi
> @@ -455,7 +460,11 @@
>
> findif_out=`$FINDIF -C`
> rc=$?
> -[ $rc -ne 0 ] && exit $rc
> +[ $rc -ne 0 ] && {
> + ocf_log err "[$FINDIF -C] failed"
> + exit $rc
> +}
> +
> INTERFACE=`echo $findif_out | awk '{print $1}'`
> NETWORK=`ip route list dev $INTERFACE scope link|grep -o '^[^ ]*'`
>

> _______________________________________________________
> Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> Home Page: http://linux-ha.org/

_______________________________________________________
Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/

Hi Dejan,

Thank you for comments.

> > This patch revises the next point.
> >
> > * When route has been already assigned, RA skips an allotment.
>
> Is this just a performance improvement? Or did you see anything
> wrong happen when running the current code?

The problem is not taking place.
We found this waste in a review.
I think that this waste may influence a performance.

>
> > * Added error log of FINDIF.
> > * Deleted the unused sentence.
>
> It would be good to have at least two patches, because we should
> always try to have patches with single self-contained
> modification.

Sorry..
Because I was small, I did not divide this patch into one patch.


Best Regards,
Hideo Yamauchi.

--- On Sat, 2012/1/14, Dejan Muhamedagic <dejan@suse.de> wrote:

> Hi Hideo-san,
>
> Sorry for picking up this so late.
>
> On Tue, Nov 29, 2011 at 02:48:52PM +0900, renayama19661014@ybb.ne.jp wrote:
> > Hi All,
> >
> > We made a patch to IPsrcaddr.
> >
> > This patch revises the next point.
> >
> >  * When route has been already assigned, RA skips an allotment.
>
> Is this just a performance improvement? Or did you see anything
> wrong happen when running the current code?
>
> >  * Added error log of FINDIF.
> >  * Deleted the unused sentence.
>
> It would be good to have at least two patches, because we should
> always try to have patches with single self-contained
> modification.
>
> Cheers,
>
> Dejan
>
> > Please please confirm my correction.
> > And please commit a correction.
> >
> > Best Regards,
> > Hideo Yamauchi
>
> > diff -r 2107bc4f5c8b heartbeat/IPsrcaddr
> > --- a/heartbeat/IPsrcaddr    Thu Nov 24 14:13:11 2011 +0900
> > +++ b/heartbeat/IPsrcaddr    Thu Nov 24 14:13:53 2011 +0900
> > @@ -167,13 +167,20 @@
> >  srca_start() {
> >      srca_read $1
> > 
> > -    ip route replace $NETWORK dev $INTERFACE src $1 || \
> > -        errorexit "command 'ip route replace $NETWORK dev $INTERFACE src $1' failed"
> > +    rc=$?
> > +    if [ $rc = 0 ]; then
> > +        rc=$OCF_SUCCESS
> > +        ocf_log info "The ip route has been already set.($NETWORK, $INTERFACE, $ROUTE_WO_SRC)"
> > +    else
> > +        ip route replace $NETWORK dev $INTERFACE src $1 || \
> > +            errorexit "command 'ip route replace $NETWORK dev $INTERFACE src $1' failed"
> > 
> > -    $CMDCHANGE $ROUTE_WO_SRC src $1 || \
> > -        errorexit "command '$CMDCHANGE $ROUTE_WO_SRC src $1' failed"
> > +        $CMDCHANGE $ROUTE_WO_SRC src $1 || \
> > +            errorexit "command '$CMDCHANGE $ROUTE_WO_SRC src $1' failed"
> > +        rc=$?
> > +    fi
> > 
> > -    return $?
> > +    return $rc
> >  }
> > 
> >  #
> > @@ -252,7 +259,6 @@
> >      else
> >          true
> >      fi       
> > -#    return $OCF_SUCCESS
> >      ;;
> >      *) #less than three decimal dots
> >      false;;
> > @@ -377,7 +383,6 @@
> >       
> >        Linux|SunOS)       
> >        IF=`find_interface "$BASEIP"`
> > -#      echo $IF
> >        if [ -z "$IF" ]; then
> >            return $OCF_NOT_RUNNING
> >        fi
> > @@ -455,7 +460,11 @@
> > 
> >  findif_out=`$FINDIF -C`
> >  rc=$?
> > -[ $rc -ne 0 ] && exit $rc
> > +[ $rc -ne 0 ] && {
> > +    ocf_log err "[$FINDIF -C] failed"
> > +    exit $rc
> > +}
> > +
> >  INTERFACE=`echo $findif_out | awk '{print $1}'`
> >  NETWORK=`ip route list dev $INTERFACE scope link|grep -o '^[^ ]*'`
> > 
>
> > _______________________________________________________
> > Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
> > http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> > Home Page: http://linux-ha.org/
>
>
_______________________________________________________
Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/

Hi Hideo-san,

On Mon, Jan 16, 2012 at 09:02:25AM +0900, renayama19661014@ybb.ne.jp wrote:
> Hi Dejan,
>
> Thank you for comments.
>
> > > This patch revises the next point.
> > >
> > > * When route has been already assigned, RA skips an allotment.
> >
> > Is this just a performance improvement? Or did you see anything
> > wrong happen when running the current code?
>
> The problem is not taking place.
> We found this waste in a review.
> I think that this waste may influence a performance.
>
> >
> > > * Added error log of FINDIF.
> > > * Deleted the unused sentence.
> >
> > It would be good to have at least two patches, because we should
> > always try to have patches with single self-contained
> > modification.
>
> Sorry..
> Because I was small, I did not divide this patch into one patch.

OK. I split them and applied.

Cheers,

Dejan


> Best Regards,
> Hideo Yamauchi.
>
> --- On Sat, 2012/1/14, Dejan Muhamedagic <dejan@suse.de> wrote:
>
> > Hi Hideo-san,
> >
> > Sorry for picking up this so late.
> >
> > On Tue, Nov 29, 2011 at 02:48:52PM +0900, renayama19661014@ybb.ne.jp wrote:
> > > Hi All,
> > >
> > > We made a patch to IPsrcaddr.
> > >
> > > This patch revises the next point.
> > >
> > >  * When route has been already assigned, RA skips an allotment.
> >
> > Is this just a performance improvement? Or did you see anything
> > wrong happen when running the current code?
> >
> > >  * Added error log of FINDIF.
> > >  * Deleted the unused sentence.
> >
> > It would be good to have at least two patches, because we should
> > always try to have patches with single self-contained
> > modification.
> >
> > Cheers,
> >
> > Dejan
> >
> > > Please please confirm my correction.
> > > And please commit a correction.
> > >
> > > Best Regards,
> > > Hideo Yamauchi
> >
> > > diff -r 2107bc4f5c8b heartbeat/IPsrcaddr
> > > --- a/heartbeat/IPsrcaddr    Thu Nov 24 14:13:11 2011 +0900
> > > +++ b/heartbeat/IPsrcaddr    Thu Nov 24 14:13:53 2011 +0900
> > > @@ -167,13 +167,20 @@
> > >  srca_start() {
> > >      srca_read $1
> > > 
> > > -    ip route replace $NETWORK dev $INTERFACE src $1 || \
> > > -        errorexit "command 'ip route replace $NETWORK dev $INTERFACE src $1' failed"
> > > +    rc=$?
> > > +    if [ $rc = 0 ]; then
> > > +        rc=$OCF_SUCCESS
> > > +        ocf_log info "The ip route has been already set.($NETWORK, $INTERFACE, $ROUTE_WO_SRC)"
> > > +    else
> > > +        ip route replace $NETWORK dev $INTERFACE src $1 || \
> > > +            errorexit "command 'ip route replace $NETWORK dev $INTERFACE src $1' failed"
> > > 
> > > -    $CMDCHANGE $ROUTE_WO_SRC src $1 || \
> > > -        errorexit "command '$CMDCHANGE $ROUTE_WO_SRC src $1' failed"
> > > +        $CMDCHANGE $ROUTE_WO_SRC src $1 || \
> > > +            errorexit "command '$CMDCHANGE $ROUTE_WO_SRC src $1' failed"
> > > +        rc=$?
> > > +    fi
> > > 
> > > -    return $?
> > > +    return $rc
> > >  }
> > > 
> > >  #
> > > @@ -252,7 +259,6 @@
> > >      else
> > >          true
> > >      fi       
> > > -#    return $OCF_SUCCESS
> > >      ;;
> > >      *) #less than three decimal dots
> > >      false;;
> > > @@ -377,7 +383,6 @@
> > >       
> > >        Linux|SunOS)       
> > >        IF=`find_interface "$BASEIP"`
> > > -#      echo $IF
> > >        if [ -z "$IF" ]; then
> > >            return $OCF_NOT_RUNNING
> > >        fi
> > > @@ -455,7 +460,11 @@
> > > 
> > >  findif_out=`$FINDIF -C`
> > >  rc=$?
> > > -[ $rc -ne 0 ] && exit $rc
> > > +[ $rc -ne 0 ] && {
> > > +    ocf_log err "[$FINDIF -C] failed"
> > > +    exit $rc
> > > +}
> > > +
> > >  INTERFACE=`echo $findif_out | awk '{print $1}'`
> > >  NETWORK=`ip route list dev $INTERFACE scope link|grep -o '^[^ ]*'`
> > > 
> >
> > > _______________________________________________________
> > > Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
> > > http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> > > Home Page: http://linux-ha.org/
> >
> >
> _______________________________________________________
> Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> Home Page: http://linux-ha.org/
_______________________________________________________
Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/

Hi Dejan,

> > Sorry..
> > Because I was small, I did not divide this patch into one patch.
>
> OK. I split them and applied.

I confirmed the reflection of the repository.
(Patch 1 and Patch 2.)

Many Thanks!!
Hideo Yamauchi.

--- On Sat, 2012/1/21, Dejan Muhamedagic <dejan@suse.de> wrote:

> Hi Hideo-san,
>
> On Mon, Jan 16, 2012 at 09:02:25AM +0900, renayama19661014@ybb.ne.jp wrote:
> > Hi Dejan,
> >
> > Thank you for comments.
> >
> > > > This patch revises the next point.
> > > >
> > > >  * When route has been already assigned, RA skips an allotment.
> > >
> > > Is this just a performance improvement? Or did you see anything
> > > wrong happen when running the current code?
> >
> > The problem is not taking place.
> > We found this waste in a review.
> > I think that this waste may influence a performance.
> >
> > >
> > > >  * Added error log of FINDIF.
> > > >  * Deleted the unused sentence.
> > >
> > > It would be good to have at least two patches, because we should
> > > always try to have patches with single self-contained
> > > modification.
> >
> > Sorry..
> > Because I was small, I did not divide this patch into one patch.
>
> OK. I split them and applied.
>
> Cheers,
>
> Dejan
>
>
> > Best Regards,
> > Hideo Yamauchi.
> >
> > --- On Sat, 2012/1/14, Dejan Muhamedagic <dejan@suse.de> wrote:
> >
> > > Hi Hideo-san,
> > >
> > > Sorry for picking up this so late.
> > >
> > > On Tue, Nov 29, 2011 at 02:48:52PM +0900, renayama19661014@ybb.ne.jp wrote:
> > > > Hi All,
> > > >
> > > > We made a patch to IPsrcaddr.
> > > >
> > > > This patch revises the next point.
> > > >
> > > >  * When route has been already assigned, RA skips an allotment.
> > >
> > > Is this just a performance improvement? Or did you see anything
> > > wrong happen when running the current code?
> > >
> > > >  * Added error log of FINDIF.
> > > >  * Deleted the unused sentence.
> > >
> > > It would be good to have at least two patches, because we should
> > > always try to have patches with single self-contained
> > > modification.
> > >
> > > Cheers,
> > >
> > > Dejan
> > >
> > > > Please please confirm my correction.
> > > > And please commit a correction.
> > > >
> > > > Best Regards,
> > > > Hideo Yamauchi
> > >
> > > > diff -r 2107bc4f5c8b heartbeat/IPsrcaddr
> > > > --- a/heartbeat/IPsrcaddr    Thu Nov 24 14:13:11 2011 +0900
> > > > +++ b/heartbeat/IPsrcaddr    Thu Nov 24 14:13:53 2011 +0900
> > > > @@ -167,13 +167,20 @@
> > > >  srca_start() {
> > > >      srca_read $1
> > > > 
> > > > -    ip route replace $NETWORK dev $INTERFACE src $1 || \
> > > > -        errorexit "command 'ip route replace $NETWORK dev $INTERFACE src $1' failed"
> > > > +    rc=$?
> > > > +    if [ $rc = 0 ]; then
> > > > +        rc=$OCF_SUCCESS
> > > > +        ocf_log info "The ip route has been already set.($NETWORK, $INTERFACE, $ROUTE_WO_SRC)"
> > > > +    else
> > > > +        ip route replace $NETWORK dev $INTERFACE src $1 || \
> > > > +            errorexit "command 'ip route replace $NETWORK dev $INTERFACE src $1' failed"
> > > > 
> > > > -    $CMDCHANGE $ROUTE_WO_SRC src $1 || \
> > > > -        errorexit "command '$CMDCHANGE $ROUTE_WO_SRC src $1' failed"
> > > > +        $CMDCHANGE $ROUTE_WO_SRC src $1 || \
> > > > +            errorexit "command '$CMDCHANGE $ROUTE_WO_SRC src $1' failed"
> > > > +        rc=$?
> > > > +    fi
> > > > 
> > > > -    return $?
> > > > +    return $rc
> > > >  }
> > > > 
> > > >  #
> > > > @@ -252,7 +259,6 @@
> > > >      else
> > > >          true
> > > >      fi       
> > > > -#    return $OCF_SUCCESS
> > > >      ;;
> > > >      *) #less than three decimal dots
> > > >      false;;
> > > > @@ -377,7 +383,6 @@
> > > >       
> > > >        Linux|SunOS)       
> > > >        IF=`find_interface "$BASEIP"`
> > > > -#      echo $IF
> > > >        if [ -z "$IF" ]; then
> > > >            return $OCF_NOT_RUNNING
> > > >        fi
> > > > @@ -455,7 +460,11 @@
> > > > 
> > > >  findif_out=`$FINDIF -C`
> > > >  rc=$?
> > > > -[ $rc -ne 0 ] && exit $rc
> > > > +[ $rc -ne 0 ] && {
> > > > +    ocf_log err "[$FINDIF -C] failed"
> > > > +    exit $rc
> > > > +}
> > > > +
> > > >  INTERFACE=`echo $findif_out | awk '{print $1}'`
> > > >  NETWORK=`ip route list dev $INTERFACE scope link|grep -o '^[^ ]*'`
> > > > 
> > >
> > > > _______________________________________________________
> > > > Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
> > > > http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> > > > Home Page: http://linux-ha.org/
> > >
> > >
> > _______________________________________________________
> > Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
> > http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> > Home Page: http://linux-ha.org/
>
_______________________________________________________
Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/