Re: [PATCH odhcpd 2/2] router: always check ra_lifetime
On Mon, Feb 13, 2023 at 9:18 PM wrote: > > @@ -495,13 +503,10 @@ static int send_router_advert(struct interface *iface, > const struct in6_addr *fr > memcpy(addrs, iface->addr6, sizeof(*addrs) * > valid_addr_cnt); > > /* Check default route */ > - if (iface->default_router) { > - default_route = true; > - > - if (iface->default_router > 1) > - valid_prefix = true; > - } else if (parse_routes(addrs, valid_addr_cnt)) > - default_route = true; > + if (!default_route) { > + if (parse_routes(addrs, valid_addr_cnt)) > + default_route = true; > + } > } > > if (invalid_addr_cnt) { Patch looks fine to me, I have only one suggestion: merge these 2 if statements into one. ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[PATCH odhcpd 2/2] router: always check ra_lifetime
We currently only check ra_default when an interface has valid addresses. This results in ra_default being ignored in case we have an interface with only link-local addresses. This effectively breaks the use of value 2 for the ra_default parameter. Fix this by always checking ra_lifetime, regardless of the interface having public addresses or not. Fixes: #11930 Fixes: 83e14f455817 ("router: advertise removed addresses as invalid in 3 consecutive RAs") Signed-off-by: Stijn Tintel --- src/router.c | 19 --- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/router.c b/src/router.c index 1c11849..21674f6 100644 --- a/src/router.c +++ b/src/router.c @@ -488,6 +488,14 @@ static int send_router_advert(struct interface *iface, const struct in6_addr *fr valid_addr_cnt = (iface->timer_rs.cb /* if not shutdown */ ? iface->addr6_len : 0); invalid_addr_cnt = iface->invalid_addr6_len; + // check ra_default + if (iface->default_router) { + default_route = true; + + if (iface->default_router > 1) + valid_prefix = true; + } + if (valid_addr_cnt + invalid_addr_cnt) { addrs = alloca(sizeof(*addrs) * (valid_addr_cnt + invalid_addr_cnt)); @@ -495,13 +503,10 @@ static int send_router_advert(struct interface *iface, const struct in6_addr *fr memcpy(addrs, iface->addr6, sizeof(*addrs) * valid_addr_cnt); /* Check default route */ - if (iface->default_router) { - default_route = true; - - if (iface->default_router > 1) - valid_prefix = true; - } else if (parse_routes(addrs, valid_addr_cnt)) - default_route = true; + if (!default_route) { + if (parse_routes(addrs, valid_addr_cnt)) + default_route = true; + } } if (invalid_addr_cnt) { -- 2.39.1 ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[PATCH odhcpd 1/2] router: improve RA logging
We only set the RA lifetime to what is configured in UCI when there is a default route and valid prefix. In any other case, we set it to 0. This leads to confusion where people believe ra_lifetime is completely ignored. In case there is a default route, but no valid prefix, a debug message explains this, but if there is no default route, we silently override ra_lifetime. Add a debug message for the latter case, and explicitly mention overriding ra_lifetime in both cases. Signed-off-by: Stijn Tintel --- src/router.c | 20 +++- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/router.c b/src/router.c index 949cbe7..1c11849 100644 --- a/src/router.c +++ b/src/router.c @@ -618,17 +618,19 @@ static int send_router_advert(struct interface *iface, const struct in6_addr *fr msecs = calc_adv_interval(iface, minvalid, ); lifetime = calc_ra_lifetime(iface, maxival); - if (default_route) { - if (!valid_prefix) { - syslog(LOG_WARNING, "A default route is present but there is no public prefix " - "on %s thus we don't announce a default route!", iface->name); - adv.h.nd_ra_router_lifetime = 0; - } else - adv.h.nd_ra_router_lifetime = htons(lifetime < UINT16_MAX ? lifetime : UINT16_MAX); - - } else + if (default_route && valid_prefix) { + adv.h.nd_ra_router_lifetime = htons(lifetime < UINT16_MAX ? lifetime : UINT16_MAX); + } else { adv.h.nd_ra_router_lifetime = 0; + if (default_route) { + syslog(LOG_WARNING, "A default route is present but there is no public prefix " + "on %s thus we don't announce a default route by overriding ra_lifetime!", iface->name); + } else { + syslog(LOG_WARNING, "No default route present, overriding ra_lifetime!"); + } + } + syslog(LOG_DEBUG, "Using a RA lifetime of %d seconds on %s", ntohs(adv.h.nd_ra_router_lifetime), iface->name); /* DNS options */ -- 2.39.1 ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: MT7621 NAND OOB misdetect
On 2/11/23 08:10, Chuanhong Guo wrote: Hi! # nanddump -a /dev/mtd2 ECC failed: 8 ECC corrected: 0 Number of bad blocks: 0 Number of bbt blocks: 0 Block size 131072, page size 2048, OOB size 128 Dumping data starting at 0x and ending at 0x0004... libmtd: error!: MEMGETBADBLOCK ioctl failed for eraseblock 0 (mtd2) error 77 (Bad message) nanddump: error!: libmtd: mtd_is_bad I haven't been able to find anything on what this error means in practice. You could try printing the spare size and ecc strength used in the old driver, replacing the calculation in the new driver with hard-coded values and see if that works. If it works, you can implement the ecc strength override in our driver. Thanks. I'm not real familiar with this, so it's slow going. I'm sure the answer is simple. Here's some more info from u-boot: # MTK NAND # : Use HW ECC NAND ID [C2 F1 80 91 03] Device found in MTK table, ID: c2f1, EXT_ID: 809103 Support this Device in MTK table! c2f1 select_chip [NAND]select ecc bit:12, sparesize :112 spare_per_sector=28 Signature matched and data read! load_fact_bbt success 1023 load fact bbt success [mtk_nand] probe successfully! mtd->writesize=2048 mtd->oobsize=112, mtd->erasesize=131072 devinfo.iowidth=8 From the vendor mtk_nand2 driver: nand_chip->ecc.mode = hw->nand_ecc_mode;/* enable ECC */ nand_chip->ecc.strength = 1; [9.398860] [NAND]select ecc bit:12, sparesize :112 spare_per_sector=28 Also note: mtd->oobsize = devinfo.sparesize; Which might be the misreporting. In our driver, it comes out as: [ 16.091826] nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 128 [ 16.107083] mt7621-nand 1e003000.nand: ECC strength adjusted to 12 bits I tried adjusting in nand_onfi.c ecc_bits = 12 and spare_bytes_per_page to 112, to no avail. I set that back, and then in mt7621_nfc_calc_ecc_strength tried setting the strength to 1, with no obvious difference. What to try next, thanks! ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH] ubus: added ubus_handle_events function that "guaranties" execution of all polled events
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software.--- Begin Message --- Hi, > You're already using a foreign event loop / IO notification mechanism, you > already have means to determine socket read readiness. Invoking a library > function that does it's own polling internally with arbitrary, uncontrollable > timeouts does not seem like a good design. Maybe, I don't get something right, but it seems like in my patch It does not do any internal polling when calling ubus_handle_events. It will call ubus_handle_data function, which is the command block ctx->sock.cb that will be executed with ctx->cancel_poll set to false. The ctx->cancel_poll could be redundant in the loop, because I don't see any way its state could be changed during the loop flow. > It would be better to implement a function that simply keeps calling > `get_next_msg(ctx, _fd)` and `ubus_process_msg(ctx, >msgbuf, > recv_fd);` until `get_next_msg()` yields false. ubus_handle_data seems to do exactly what you've described, so is there any other reason to implement separate function? From: openwrt-devel on behalf of Jo-Philipp Wich Sent: Monday, February 13, 2023 3:12 PM To: openwrt-devel@lists.openwrt.org Subject: Re: [PATCH] ubus: added ubus_handle_events function that "guaranties" execution of all polled events [EXTERNAL]-Real sender is: openwrt-devel-bounces+taras.i.boichuk-ext=sagemcom@lists.openwrt.org -- Hi, > In case of previous setup or calling flow ctx->cancel_poll is set to true > function ubus_handle_event may process ONLY ONE request, though the comment > says it processes events: > > /* call this for read events on ctx->sock.fd when not using uloop */ static > inline void ubus_handle_event(struct ubus_context *ctx) { > ctx->sock.cb(>sock, ULOOP_READ); } > > In case if I would manually poll the ubus fd and do not use uloop to poll > it and after that it may process ONE event and the rest will be processed > on the next loop cycle. I would like to have a function that guarantees > that every request will be processed in a single call to > ubus_haubus_handle_eventndle_event. You're already using a foreign event loop / IO notification mechanism, you already have means to determine socket read readiness. Invoking a library function that does it's own polling internally with arbitrary, uncontrollable timeouts does not seem like a good design. It would be better to implement a function that simply keeps calling `get_next_msg(ctx, _fd)` and `ubus_process_msg(ctx, >msgbuf, recv_fd);` until `get_next_msg()` yields false. ~ Jo __ Ce courriel et les documents qui lui sont joints sont, sauf mention contraire, présumés de nature confidentielle et destinées à l'usage exclusif du ou des destinataire(s) mentionné(s). Si vous n'êtes pas le ou les destinataire(s), vous êtes informé(e) que toute divulgation, reproduction, distribution, toute autre diffusion ou utilisation de cette communication ou de tout ou partie de ces informations est strictement interdite, sauf accord préalable de l’expéditeur. Si ce message vous a été transmis par erreur, merci d’immédiatement en informer l'expéditeur et supprimer de votre système informatique ce courriel ainsi que tous les documents qui y sont attachés. En vous remerciant de votre coopération. This email and any attached documents are, unless otherwise stated, presumed to be confidential and intended for the exclusive use of the recipient(s) mentioned. If you are not the recipient(s), you are informed that any disclosure, reproduction, distribution, any other dissemination or use of this communication or all or part of this information is strictly prohibited, unless agreed beforehand by the sender. If you have received this e-mail in error, please immediately advise the sender and delete this e-mail and all the attached documents from your computer system. Thanking you for your cooperation. --- End Message --- ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH] ubus: added ubus_handle_events function that "guaranties" execution of all polled events
Hi, > In case of previous setup or calling flow ctx->cancel_poll is set to true > function ubus_handle_event may process ONLY ONE request, though the comment > says it processes events: > > /* call this for read events on ctx->sock.fd when not using uloop */ static > inline void ubus_handle_event(struct ubus_context *ctx) { > ctx->sock.cb(>sock, ULOOP_READ); } > > In case if I would manually poll the ubus fd and do not use uloop to poll > it and after that it may process ONE event and the rest will be processed > on the next loop cycle. I would like to have a function that guarantees > that every request will be processed in a single call to > ubus_haubus_handle_eventndle_event. You're already using a foreign event loop / IO notification mechanism, you already have means to determine socket read readiness. Invoking a library function that does it's own polling internally with arbitrary, uncontrollable timeouts does not seem like a good design. It would be better to implement a function that simply keeps calling `get_next_msg(ctx, _fd)` and `ubus_process_msg(ctx, >msgbuf, recv_fd);` until `get_next_msg()` yields false. ~ Jo signature.asc Description: OpenPGP digital signature ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH] realtek: fix memory leak in netevent handler
Hi Jan, On Wed, 2023-02-08 at 22:53 +0100, Jan Hoffmann wrote: > The net_event_work struct is allocated, but only freed in a single case. > Move the allocation to the branch where it is actually needed, and free > it after the work has been done. > > Fixes: 03e1d93e0779 ("realtek: add driver support for routing offload") > Signed-off-by: Jan Hoffmann > --- > I noticed this issue due to increasing memory usage on a HPE 1920-16G. > While I haven't done any long-term testing with this patch yet, using > kmemleak on a HPE 1920-8G no longer shows any leaks in the realtek > drivers. Thanks for noticing and taking care of fixing it! > > .../files-5.10/drivers/net/dsa/rtl83xx/common.c | 17 + > .../files-5.15/drivers/net/dsa/rtl83xx/common.c | 17 + > 2 files changed, 18 insertions(+), 16 deletions(-) > > diff --git a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c > b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c > index 15e6ed092696..d2d677230010 100644 > --- a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c > +++ b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c > @@ -1286,6 +1286,8 @@ static void rtl83xx_net_event_work_do(struct > work_struct *work) > struct rtl838x_switch_priv *priv = net_work->priv; > > rtl83xx_l3_nexthop_update(priv, net_work->gw_addr, net_work->mac); > + > + kfree(net_work); > } > > static int rtl83xx_netevent_event(struct notifier_block *this, > @@ -1299,13 +1301,6 @@ static int rtl83xx_netevent_event(struct > notifier_block *this, > > priv = container_of(this, struct rtl838x_switch_priv, ne_nb); > > - net_work = kzalloc(sizeof(*net_work), GFP_ATOMIC); > - if (!net_work) > - return NOTIFY_BAD; > - > - INIT_WORK(_work->work, rtl83xx_net_event_work_do); > - net_work->priv = priv; > - > switch (event) { > case NETEVENT_NEIGH_UPDATE: > if (n->tbl != _tbl) > @@ -1314,10 +1309,16 @@ static int rtl83xx_netevent_event(struct > notifier_block *this, > port = rtl83xx_port_dev_lower_find(dev, priv); > if (port < 0 || !(n->nud_state & NUD_VALID)) { > pr_debug("%s: Neigbour invalid, not updating\n", > __func__); > - kfree(net_work); > return NOTIFY_DONE; > } > > + net_work = kzalloc(sizeof(*net_work), GFP_ATOMIC); > + if (!net_work) > + return NOTIFY_BAD; > + > + INIT_WORK(_work->work, rtl83xx_net_event_work_do); > + net_work->priv = priv; > + > net_work->mac = ether_addr_to_u64(n->ha); > net_work->gw_addr = *(__be32 *) n->primary_key; rtl83xx_netevent_event() has a variable err which isn't ever assigned, so I think there's a bit of dead code at the end. Otherwise I think this change is good, and err could be removed in another patch. Best, Sander ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[PATCH] ubus: added ubus_handle_events function that "guaranties" execution of all polled events
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software.--- Begin Message --- In case of previous setup or calling flow ctx->cancel_poll is set to true function ubus_handle_event may process ONLY ONE request, though the comment says it processes events: /* call this for read events on ctx->sock.fd when not using uloop */ static inline void ubus_handle_event(struct ubus_context *ctx) { ctx->sock.cb(>sock, ULOOP_READ); } In case if I would manually poll the ubus fd and do not use uloop to poll it and after that it may process ONE event and the rest will be processed on the next loop cycle. I would like to have a function that guarantees that every request will be processed in a single call to ubus_haubus_handle_eventndle_event. I suggest creating another function that will do this: /* call this for read ALL events on ctx->sock.fd when not using uloop * after polling fd manually */ static inline void ubus_handle_events(struct ubus_context *ctx) { ctx->cancel_poll = false; ctx->sock.cb(>sock, ULOOP_READ); } This idea comes from existing not exposed functionality of the function that does the polling by itself and then processes all messages: void __hidden ubus_poll_data(struct ubus_context *ctx, int timeout) { struct pollfd pfd = { .fd = ctx->sock.fd, .events = POLLIN | POLLERR, }; ctx->cancel_poll = false; poll(, 1, timeout ? timeout : -1); ubus_handle_data(>sock, ULOOP_READ); } Instead of letting it do the polling we will do it our-self. Please, review and advice. A few words regrading the intention of this add-on: I am using the system that has a few different fds that are being polled and events on each are processed. Sometimes there are more work on the fds, so processing ubus events take a lot of time with ubus_handle_event. Additionally due to previous code that sets up ubus object ctx->cancel_poll becomes true. __ Ce courriel et les documents qui lui sont joints sont, sauf mention contraire, présumés de nature confidentielle et destinées à l'usage exclusif du ou des destinataire(s) mentionné(s). Si vous n'êtes pas le ou les destinataire(s), vous êtes informé(e) que toute divulgation, reproduction, distribution, toute autre diffusion ou utilisation de cette communication ou de tout ou partie de ces informations est strictement interdite, sauf accord préalable de l’expéditeur. Si ce message vous a été transmis par erreur, merci d’immédiatement en informer l'expéditeur et supprimer de votre système informatique ce courriel ainsi que tous les documents qui y sont attachés. En vous remerciant de votre coopération. This email and any attached documents are, unless otherwise stated, presumed to be confidential and intended for the exclusive use of the recipient(s) mentioned. If you are not the recipient(s), you are informed that any disclosure, reproduction, distribution, any other dissemination or use of this communication or all or part of this information is strictly prohibited, unless agreed beforehand by the sender. If you have received this e-mail in error, please immediately advise the sender and delete this e-mail and all the attached documents from your computer system. Thanking you for your cooperation. diff --git a/libubus.h b/libubus.h index dc42ea7..b4b7140 100644 --- a/libubus.h +++ b/libubus.h @@ -260,6 +260,15 @@ static inline void ubus_add_uloop(struct ubus_context *ctx) uloop_fd_add(>sock, ULOOP_BLOCKING | ULOOP_READ); } +/* call this for read ALL events on ctx->sock.fd when not using uloop + * after polling fd manually + */ +static inline void ubus_handle_events(struct ubus_context *ctx) +{ + ctx->cancel_poll = false; + ctx->sock.cb(>sock, ULOOP_READ); +} + /* call this for read events on ctx->sock.fd when not using uloop */ static inline void ubus_handle_event(struct ubus_context *ctx) { @@ -380,7 +389,7 @@ static inline int ubus_request_get_caller_fd(struct ubus_request_data *req) { int fd = req->req_fd; req->req_fd = -1; - + return fd; } --- End Message --- ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[PATCH] netifd: refactor packet steering
From: Rafał Miłecki 1. Move setup code to independent script file 2. Add init.d script to allow reload_config 3. Support platform specific /lib/platform/packet-steering.sh Signed-off-by: Rafał Miłecki --- package/network/config/netifd/Makefile| 2 +- .../etc/hotplug.d/net/20-smp-packet-steering | 67 +- .../netifd/files/etc/init.d/packet_steering | 17 + .../files/lib/network/packet-steering.sh | 70 +++ 4 files changed, 89 insertions(+), 67 deletions(-) create mode 100644 package/network/config/netifd/files/etc/init.d/packet_steering create mode 100755 package/network/config/netifd/files/lib/network/packet-steering.sh diff --git a/package/network/config/netifd/Makefile b/package/network/config/netifd/Makefile index 500daaa152..f40a990b42 100644 --- a/package/network/config/netifd/Makefile +++ b/package/network/config/netifd/Makefile @@ -1,7 +1,7 @@ include $(TOPDIR)/rules.mk PKG_NAME:=netifd -PKG_RELEASE:=1 +PKG_RELEASE:=2 PKG_SOURCE_PROTO:=git PKG_SOURCE_URL=$(PROJECT_GIT)/project/netifd.git diff --git a/package/network/config/netifd/files/etc/hotplug.d/net/20-smp-packet-steering b/package/network/config/netifd/files/etc/hotplug.d/net/20-smp-packet-steering index 8a86bf75f6..576f244945 100644 --- a/package/network/config/netifd/files/etc/hotplug.d/net/20-smp-packet-steering +++ b/package/network/config/netifd/files/etc/hotplug.d/net/20-smp-packet-steering @@ -1,67 +1,2 @@ #!/bin/sh -[ "$ACTION" = add ] || exit - -NPROCS="$(grep -c "^processor.*:" /proc/cpuinfo)" -[ "$NPROCS" -gt 1 ] || exit - -PROC_MASK="$(( (1 << $NPROCS) - 1 ))" - -find_irq_cpu() { - local dev="$1" - local match="$(grep -m 1 "$dev\$" /proc/interrupts)" - local cpu=0 - - [ -n "$match" ] && { - set -- $match - shift - for cur in $(seq 1 $NPROCS); do - [ "$1" -gt 0 ] && { - cpu=$(($cur - 1)) - break - } - shift - done - } - - echo "$cpu" -} - -set_hex_val() { - local file="$1" - local val="$2" - val="$(printf %x "$val")" - [ -n "$DEBUG" ] && echo "$file = $val" - echo "$val" > "$file" -} - -packet_steering="$(uci get "network.@globals[0].packet_steering")" -[ "$packet_steering" != 1 ] && exit 0 - -exec 512>/var/lock/smp_tune.lock -flock 512 || exit 1 - -for dev in /sys/class/net/*; do - [ -d "$dev" ] || continue - - # ignore virtual interfaces - [ -n "$(ls "${dev}/" | grep '^lower_')" ] && continue - [ -d "${dev}/device" ] || continue - - device="$(readlink "${dev}/device")" - device="$(basename "$device")" - irq_cpu="$(find_irq_cpu "$device")" - irq_cpu_mask="$((1 << $irq_cpu))" - - for q in ${dev}/queues/tx-*; do - set_hex_val "$q/xps_cpus" "$PROC_MASK" - done - - # ignore dsa slave ports for RPS - subsys="$(readlink "${dev}/device/subsystem")" - subsys="$(basename "$subsys")" - [ "$subsys" = "mdio_bus" ] && continue - - for q in ${dev}/queues/rx-*; do - set_hex_val "$q/rps_cpus" "$PROC_MASK" - done -done +[ "$ACTION" = add ] && /lib/network/packet-steering.sh diff --git a/package/network/config/netifd/files/etc/init.d/packet_steering b/package/network/config/netifd/files/etc/init.d/packet_steering new file mode 100644 index 00..94229998fd --- /dev/null +++ b/package/network/config/netifd/files/etc/init.d/packet_steering @@ -0,0 +1,17 @@ +#!/bin/sh /etc/rc.common + +START=25 +USE_PROCD=1 + +start_service() { + reload_service +} + +service_triggers() { + procd_add_reload_trigger "network" + procd_add_reload_trigger "firewall" +} + +reload_service() { + /lib/network/packet-steering.sh +} diff --git a/package/network/config/netifd/files/lib/network/packet-steering.sh b/package/network/config/netifd/files/lib/network/packet-steering.sh new file mode 100755 index 00..088c631046 --- /dev/null +++ b/package/network/config/netifd/files/lib/network/packet-steering.sh @@ -0,0 +1,70 @@ +#!/bin/sh +NPROCS="$(grep -c "^processor.*:" /proc/cpuinfo)" +[ "$NPROCS" -gt 1 ] || exit + +PROC_MASK="$(( (1 << $NPROCS) - 1 ))" + +find_irq_cpu() { + local dev="$1" + local match="$(grep -m 1 "$dev\$" /proc/interrupts)" + local cpu=0 + + [ -n "$match" ] && { + set -- $match + shift + for cur in $(seq 1 $NPROCS); do + [ "$1" -gt 0 ] && { + cpu=$(($cur - 1)) + break + } + shift + done + } + + echo "$cpu" +} + +set_hex_val() { + local file="$1" + local val="$2" + val="$(printf %x "$val")" + [ -n "$DEBUG" ] && echo "$file = $val" + echo