Mailing List Archive: Re: [Pacemaker] A couple of SendArp resource changes

Hello,

Anybody have objections to the patches posted here? If not, I'll
push them upstream.

Cheers,

Dejan

On Wed, Mar 13, 2013 at 07:01:33PM +0100, Dejan Muhamedagic wrote:
> Hi,
>
> On Sat, Mar 09, 2013 at 07:53:34PM +0000, Tim Small wrote:
> > Hi,
> >
> > I've been using the ocf:heartbeat:SendArp script and notice a couple of
> > issues - some problems with starting and monitoring the service, and
> > also a file descriptor leak in the binary (which would cause it to
> > terminate).
> >
> > I've detailed the problems and supplied some patches:
> >
> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=701913
>
> Cannot just replace the whole RA. Sorry. If you could split the
> patch we can consider them on a one-by-one basis. Otherwise, I
> found some patch in my local queue, which never got pushed for
> some reason. Don't know if that would help (attached).
>
> > and
> >
> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=701914
>
> Can you try the attached send_arp.libnet.c patch. It does first
> packet build then reuses them.
>
> Cheers,
>
> Dejan
>
> > ... they're not perfect, but an improvement I think.
> >
> > HTH,
> >
> > Tim.
> >
> > --
> > South East Open Source Solutions Limited
> > Registered in England and Wales with company number 06134732.
> > Registered Office: 2 Powell Gardens, Redhill, Surrey, RH1 1TQ
> > VAT number: 900 6633 53 http://seoss.co.uk/ +44-(0)1273-808309
> >
> >
> > _______________________________________________
> > Pacemaker mailing list: Pacemaker@oss.clusterlabs.org
> > http://oss.clusterlabs.org/mailman/listinfo/pacemaker
> >
> > Project Home: http://www.clusterlabs.org
> > Getting started: http://www.clusterlabs.org/doc/Cluster_from_Scratch.pdf
> > Bugs: http://bugs.clusterlabs.org

> From 9dae34616ef62b98b762a1f821f9e1ee749e6315 Mon Sep 17 00:00:00 2001
> From: Dejan Muhamedagic <dejan@suse.de>
> Date: Wed, 13 Mar 2013 18:19:10 +0100
> Subject: [PATCH] Medium: tools: send_arp.libnet: reuse ARP packets
>
> ---
> tools/send_arp.libnet.c | 174 ++++++++++++++++++++++++++++++++----------------
> 1 file changed, 115 insertions(+), 59 deletions(-)
>
> diff --git a/tools/send_arp.libnet.c b/tools/send_arp.libnet.c
> index 71148bb..2abeecb 100644
> --- a/tools/send_arp.libnet.c
> +++ b/tools/send_arp.libnet.c
> @@ -49,17 +49,18 @@
>
> #ifdef HAVE_LIBNET_1_0_API
> # define LTYPE struct libnet_link_int
> + static u_char *mk_packet(u_int32_t ip, u_char *device, u_char *macaddr, u_char *broadcast, u_char *netmask, u_short arptype);
> + static int send_arp(struct libnet_link_int *l, u_char *device, u_char *buf);
> #endif
> #ifdef HAVE_LIBNET_1_1_API
> # define LTYPE libnet_t
> + static libnet_t *mk_packet(libnet_t* lntag, u_int32_t ip, u_char *device, u_char macaddr[6], u_char *broadcast, u_char *netmask, u_short arptype);
> + int send_arp(libnet_t* lntag);
> #endif
>
> #define PIDDIR HA_VARRUNDIR "/" PACKAGE
> #define PIDFILE_BASE PIDDIR "/send_arp-"
>
> -static int send_arp(LTYPE* l, u_int32_t ip, u_char *device, u_char mac[6]
> -, u_char *broadcast, u_char *netmask, u_short arptype);
> -
> static char print_usage[]={
> "send_arp: sends out custom ARP packet.\n"
> " usage: send_arp [-i repeatinterval-ms] [-r repeatcount] [-p pidfile] \\\n"
> @@ -135,7 +136,6 @@ main(int argc, char *argv[])
> char* netmask;
> u_int32_t ip;
> u_char src_mac[6];
> - LTYPE* l;
> int repeatcount = 1;
> int j;
> long msinterval = 1000;
> @@ -143,6 +143,13 @@ main(int argc, char *argv[])
> char pidfilenamebuf[64];
> char *pidfilename = NULL;
>
> +#ifdef HAVE_LIBNET_1_0_API
> + LTYPE* l;
> + u_char *request, *reply;
> +#elif defined(HAVE_LIBNET_1_1_API)
> + LTYPE *request, *reply;
> +#endif
> +
> CL_SIGNAL(SIGTERM, byebye);
> CL_SIGINTERRUPT(SIGTERM, 1);
>
> @@ -201,6 +208,24 @@ main(int argc, char *argv[])
> return EXIT_FAILURE;
> }
>
> + if (!strcasecmp(macaddr, AUTO_MAC_ADDR)) {
> + if (get_hw_addr(device, src_mac) < 0) {
> + cl_log(LOG_ERR, "Cannot find mac address for %s",
> + device);
> + unlink(pidfilename);
> + return EXIT_FAILURE;
> + }
> + }
> + else {
> + convert_macaddr((unsigned char *)macaddr, src_mac);
> + }
> +
> +/*
> + * We need to send both a broadcast ARP request as well as the ARP response we
> + * were already sending. All the interesting research work for this fix was
> + * done by Masaki Hasegawa <masaki-h@pp.iij4u.or.jp> and his colleagues.
> + */
> +
> #if defined(HAVE_LIBNET_1_0_API)
> #ifdef ON_DARWIN
> if ((ip = libnet_name_resolve((unsigned char*)ipaddr, 1)) == -1UL) {
> @@ -219,49 +244,65 @@ main(int argc, char *argv[])
> unlink(pidfilename);
> return EXIT_FAILURE;
> }
> + request = mk_packet(ip, (unsigned char*)device, src_mac
> + , (unsigned char*)broadcast, (unsigned char*)netmask
> + , ARPOP_REQUEST);
> + reply = mk_packet(ip, (unsigned char*)device, src_mac
> + , (unsigned char *)broadcast
> + , (unsigned char *)netmask, ARPOP_REPLY);
> + if (!request || !reply) {
> + cl_log(LOG_ERR, "could not create packets");
> + unlink(pidfilename);
> + return EXIT_FAILURE;
> + }
> + for (j=0; j < repeatcount; ++j) {
> + c = send_arp(l, (unsigned char*)device, request);
> + if (c < 0) {
> + break;
> + }
> + mssleep(msinterval / 2);
> + c = send_arp(l, (unsigned char*)device, reply);
> + if (c < 0) {
> + break;
> + }
> + if (j != repeatcount-1) {
> + mssleep(msinterval / 2);
> + }
> + }
> #elif defined(HAVE_LIBNET_1_1_API)
> - if ((l=libnet_init(LIBNET_LINK, device, errbuf)) == NULL) {
> + if ((request=libnet_init(LIBNET_LINK, device, errbuf)) == NULL) {
> cl_log(LOG_ERR, "libnet_init failure on %s: %s", device, errbuf);
> unlink(pidfilename);
> return EXIT_FAILURE;
> }
> - if ((signed)(ip = libnet_name2addr4(l, ipaddr, 1)) == -1) {
> - cl_log(LOG_ERR, "Cannot resolve IP address [%s]", ipaddr);
> + if ((reply=libnet_init(LIBNET_LINK, device, errbuf)) == NULL) {
> + cl_log(LOG_ERR, "libnet_init failure on %s: %s", device, errbuf);
> unlink(pidfilename);
> return EXIT_FAILURE;
> }
> -#else
> -# error "Must have LIBNET API version defined."
> -#endif
> -
> - if (!strcasecmp(macaddr, AUTO_MAC_ADDR)) {
> - if (get_hw_addr(device, src_mac) < 0) {
> - cl_log(LOG_ERR, "Cannot find mac address for %s",
> - device);
> - unlink(pidfilename);
> - return EXIT_FAILURE;
> - }
> + if ((signed)(ip = libnet_name2addr4(request, ipaddr, 1)) == -1) {
> + cl_log(LOG_ERR, "Cannot resolve IP address [%s]", ipaddr);
> + unlink(pidfilename);
> + return EXIT_FAILURE;
> }
> - else {
> - convert_macaddr((unsigned char *)macaddr, src_mac);
> + request = mk_packet(request, ip, (unsigned char*)device, src_mac
> + , (unsigned char*)broadcast, (unsigned char*)netmask
> + , ARPOP_REQUEST);
> + reply = mk_packet(reply, ip, (unsigned char*)device, src_mac
> + , (unsigned char *)broadcast
> + , (unsigned char *)netmask, ARPOP_REPLY);
> + if (!request || !reply) {
> + cl_log(LOG_ERR, "could not create packets");
> + unlink(pidfilename);
> + return EXIT_FAILURE;
> }
> -
> -/*
> - * We need to send both a broadcast ARP request as well as the ARP response we
> - * were already sending. All the interesting research work for this fix was
> - * done by Masaki Hasegawa <masaki-h@pp.iij4u.or.jp> and his colleagues.
> - */
> for (j=0; j < repeatcount; ++j) {
> - c = send_arp(l, ip, (unsigned char*)device, src_mac
> - , (unsigned char*)broadcast, (unsigned char*)netmask
> - , ARPOP_REQUEST);
> + c = send_arp(request);
> if (c < 0) {
> break;
> }
> mssleep(msinterval / 2);
> - c = send_arp(l, ip, (unsigned char*)device, src_mac
> - , (unsigned char *)broadcast
> - , (unsigned char *)netmask, ARPOP_REPLY);
> + c = send_arp(reply);
> if (c < 0) {
> break;
> }
> @@ -269,6 +310,9 @@ main(int argc, char *argv[])
> mssleep(msinterval / 2);
> }
> }
> +#else
> +# error "Must have LIBNET API version defined."
> +#endif
>
> unlink(pidfilename);
> return c < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
> @@ -387,10 +431,9 @@ get_hw_addr(char *device, u_char mac[6])
> */
>
> #ifdef HAVE_LIBNET_1_0_API
> -int
> -send_arp(struct libnet_link_int *l, u_int32_t ip, u_char *device, u_char *macaddr, u_char *broadcast, u_char *netmask, u_short arptype)
> +u_char *
> +mk_packet(u_int32_t ip, u_char *device, u_char *macaddr, u_char *broadcast, u_char *netmask, u_short arptype)
> {
> - int n;
> u_char *buf;
> u_char *target_mac;
> u_char device_mac[6];
> @@ -400,7 +443,7 @@ send_arp(struct libnet_link_int *l, u_int32_t ip, u_char *device, u_char *macadd
>
> if (libnet_init_packet(LIBNET_ARP_H + LIBNET_ETH_H, &buf) == -1) {
> cl_log(LOG_ERR, "libnet_init_packet memory:");
> - return -1;
> + return NULL;
> }
>
> /* Convert ASCII Mac Address to 6 Hex Digits. */
> @@ -409,14 +452,14 @@ send_arp(struct libnet_link_int *l, u_int32_t ip, u_char *device, u_char *macadd
> if (get_hw_addr((char*)device, device_mac) < 0) {
> cl_log(LOG_ERR, "Cannot find mac address for %s",
> device);
> - return -1;
> + return NULL;
> }
>
> if (libnet_build_ethernet(bcast_mac, device_mac, ETHERTYPE_ARP, NULL, 0
> , buf) == -1) {
> cl_log(LOG_ERR, "libnet_build_ethernet failed:");
> libnet_destroy_packet(&buf);
> - return -1;
> + return NULL;
> }
>
> if (arptype == ARPOP_REQUEST) {
> @@ -426,8 +469,8 @@ send_arp(struct libnet_link_int *l, u_int32_t ip, u_char *device, u_char *macadd
> target_mac = macaddr;
> }
> else {
> - cl_log(LOG_ERR, "unkonwn arptype:");
> - return -1;
> + cl_log(LOG_ERR, "unknown arptype");
> + return NULL;
> }
>
> /*
> @@ -447,16 +490,9 @@ send_arp(struct libnet_link_int *l, u_int32_t ip, u_char *device, u_char *macadd
> buf + LIBNET_ETH_H) == -1) {
> cl_log(LOG_ERR, "libnet_build_arp failed:");
> libnet_destroy_packet(&buf);
> - return -1;
> + return NULL;
> }
> -
> - n = libnet_write_link_layer(l, (char*)device, buf, LIBNET_ARP_H + LIBNET_ETH_H);
> - if (n == -1) {
> - cl_log(LOG_ERR, "libnet_build_ethernet failed:");
> - }
> -
> - libnet_destroy_packet(&buf);
> - return (n);
> + return buf;
> }
> #endif /* HAVE_LIBNET_1_0_API */
>
> @@ -464,10 +500,9 @@ send_arp(struct libnet_link_int *l, u_int32_t ip, u_char *device, u_char *macadd
>
>
> #ifdef HAVE_LIBNET_1_1_API
> -int
> -send_arp(libnet_t* lntag, u_int32_t ip, u_char *device, u_char macaddr[6], u_char *broadcast, u_char *netmask, u_short arptype)
> +libnet_t*
> +mk_packet(libnet_t* lntag, u_int32_t ip, u_char *device, u_char macaddr[6], u_char *broadcast, u_char *netmask, u_short arptype)
> {
> - int n;
> u_char *target_mac;
> u_char device_mac[6];
> u_char bcast_mac[6] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> @@ -481,7 +516,7 @@ send_arp(libnet_t* lntag, u_int32_t ip, u_char *device, u_char macaddr[6], u_cha
> }
> else {
> cl_log(LOG_ERR, "unkonwn arptype:");
> - return -1;
> + return NULL;
> }
>
> /*
> @@ -502,28 +537,49 @@ send_arp(libnet_t* lntag, u_int32_t ip, u_char *device, u_char macaddr[6], u_cha
> 0 /* packet id */
> ) == -1 ) {
> cl_log(LOG_ERR, "libnet_build_arp failed:");
> - return -1;
> + return NULL;
> }
>
> /* Ethernet header */
> if (get_hw_addr((char *)device, device_mac) < 0) {
> cl_log(LOG_ERR, "Cannot find mac address for %s",
> device);
> - return -1;
> + return NULL;
> }
>
> if (libnet_build_ethernet(bcast_mac, device_mac, ETHERTYPE_ARP, NULL, 0
> , lntag, 0) == -1 ) {
> cl_log(LOG_ERR, "libnet_build_ethernet failed:");
> - return -1;
> + return NULL;
> }
> + return lntag;
> +}
> +#endif /* HAVE_LIBNET_1_1_API */
>
> - n = libnet_write(lntag);
> +#ifdef HAVE_LIBNET_1_0_API
> +int
> +send_arp(struct libnet_link_int *l, u_char *device, u_char *buf)
> +{
> + int n;
> +
> + n = libnet_write_link_layer(l, (char*)device, buf, LIBNET_ARP_H + LIBNET_ETH_H);
> if (n == -1) {
> - cl_log(LOG_ERR, "libnet_build_ethernet failed:");
> + cl_log(LOG_ERR, "libnet_write_link_layer failed");
> }
> - libnet_clear_packet(lntag);
> + return (n);
> +}
> +#endif /* HAVE_LIBNET_1_0_API */
>
> +#ifdef HAVE_LIBNET_1_1_API
> +int
> +send_arp(libnet_t* lntag)
> +{
> + int n;
> +
> + n = libnet_write(lntag);
> + if (n == -1) {
> + cl_log(LOG_ERR, "libnet_write failed");
> + }
> return (n);
> }
> #endif /* HAVE_LIBNET_1_1_API */
> --
> 1.8.0
>

> diff --git a/heartbeat/SendArp b/heartbeat/SendArp
> index 553469b..611bb47 100755
> --- a/heartbeat/SendArp
> +++ b/heartbeat/SendArp
> @@ -123,19 +123,6 @@ usage: $0 {start|stop|monitor|validate-all|meta-data}
> Expects to have a fully populated OCF RA-compliant environment set.
> END
> }
> -#
> -# Check if ip-address is running.
> -# We return stopped or running.
> -#
> -sendarp_status() {
> - if
> - [ -f "$SENDARPPIDFILE" ]
> - then
> - return $OCF_SUCCESS
> - else
> - return $OCF_NOT_RUNNING
> - fi
> -}
>
> #
> # Send gratuitous arp
> @@ -144,12 +131,12 @@ sendarp_start() {
>
> sendarp_validate
> if [ $? = $OCF_ERR_ARGS ]; then
> - return $OCF_ERR_ARGS;
> + return $OCF_ERR_ARGS
> fi
>
> - sendarp_status
> + sendarp_monitor
> if [ $? = $OCF_SUCCESS ]; then
> - return $OCF_SUCCESS;
> + return $OCF_SUCCESS
> fi
>
> [ -r ${HA_CONFDIR}/arp_config ] && . ${HA_CONFDIR}/arp_config
> @@ -169,20 +156,21 @@ sendarp_start() {
> $SENDARP $ARGS || (ocf_log err "Could not send gratuitous arps"; return $OCF_ERR_GENERIC)
> ;;
> esac
> - return $OCF_SUCCESS;
> + ha_pseudo_resource SendArp_${OCF_RESOURCE_INSTANCE} start
> + return $OCF_SUCCESS
> }
>
> #
> # Stop sending gratuitous arp
> #
> sendarp_stop() {
> - sendarp_validate
> - if [ $? = $OCF_ERR_ARGS ]; then
> - return $OCF_ERR_ARGS;
> + sendarp_monitor
> + if [ $? -eq $OCF_NOT_RUNNING ]; then
> + return $OCF_SUCCESS
> fi
> -
> +
> rc=$OCF_SUCCESS
> -
> +
> if
> [ -f "$SENDARPPIDFILE" ]
> then
> @@ -204,6 +192,7 @@ sendarp_stop() {
> case $rc in
> $OCF_SUCCESS)
> ocf_log info "SendArp for $BASEIP/$INTERFACE released"
> + ha_pseudo_resource SendArp_${OCF_RESOURCE_INSTANCE} stop
> ;;
> *)
> ocf_log warn "SendArp for $BASEIP/$INTERFACE NOT released"
> @@ -215,7 +204,7 @@ sendarp_stop() {
> # This is always active, because it doesn't do much
> #
> sendarp_monitor() {
> - return $OCF_SUCCESS
> + ha_pseudo_resource SendArp_${OCF_RESOURCE_INSTANCE} monitor
> }
>
> sendarp_validate() {
> @@ -236,7 +225,7 @@ stop) sendarp_stop
> ;;
> monitor) sendarp_monitor
> ;;
> -status) sendarp_status
> +status) sendarp_monitor
> if [ $? = $OCF_SUCCESS ]; then
> echo "running"
> exit $OCF_SUCCESS;

> _______________________________________________________
> 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/

On Mon, Apr 08, 2013 at 02:33:27PM +0100, Tim Small wrote:
> On 13/03/13 18:01, Dejan Muhamedagic wrote:
>
> > If you could split the
> > patch we can consider them on a one-by-one basis.
>
> I used Debian's "start-stop-daemon" utility in my modified script, and
> it looks like Redhat etc. doesn't package it (yet):
> http://fedoraproject.org/wiki/Features/start-stop-daemon
>
> ... the comments in that page express why I chose to use
> start-stop-daemon - reworking the script to have the same level of
> functionality as the start-stop-daemon version (but just using lsb
> stuff) would be a bit awkward + time-consuming.
>
> How about I use start-stop-daemon where available, and the LSB functions
> when not? This would still represent an improvement on the current
> behaviour of the script - which is pretty broken - e.g. stopping an
> already-stopped resource fails, and stuff like this:
>
>
> #
> # This is always active, because it doesn't do much
> #
> sendarp_monitor() {
> return $OCF_SUCCESS
> }
>
>
>
> and this:
>
>
>
> sendarp_status() {
> if
> [ -f "$SENDARPPIDFILE" ]
> then
> return $OCF_SUCCESS
> else
> return $OCF_NOT_RUNNING
> fi
> }
>
>
> "A pid file is there, so it must be running!"

The fix for the resource agent itself is already in the
repository. It is based on the standard ha_pseudo_* functions
like in any other pseudo agents (i.e. those that don't have long
running processes).

> > Otherwise, I found some patch in my local queue, which never got pushed for some reason. Don't know if that would help (attached).
> >
>
> I'll have a go with them, and check to see if they fix the bug which I
> was seeing.

Did you get a chance to verify the two patches attached? There's
now also a pull request for the socket leaks issue at github.com:

https://github.com/ClusterLabs/resource-agents/pull/247

Cheers,

Dejan

> Tim.
>
>
> --
> South East Open Source Solutions Limited
> Registered in England and Wales with company number 06134732.
> Registered Office: 2 Powell Gardens, Redhill, Surrey, RH1 1TQ
> VAT number: 900 6633 53 http://seoss.co.uk/ +44-(0)1273-808309
>
> _______________________________________________________
> 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/

Mailing List Archive

Mailing List Archive

Attached Files: