Ticket #2401 (closed defect: fixed)

Opened 6 years ago

Last modified 5 years ago

Problems with ftp on NAS synology devices based on Marvell mv5281 ARM processor

Reported by: zyxmon Owned by: zaytsev
Priority: major Milestone: 4.8.0-pre1
Component: mc-vfs Version: master
Keywords: NetBSD ftpfs Cc: cheusov
Blocked By: Blocking:
Branch state: Votes for changeset: committed-master committed-stable

Description

I am not sure it is a bug.
Here is a small report.
I have crosscompiled mc for different platforms (cpu's) for NAS synology using patch from ticket http://www.midnight-commander.org/ticket/2282

One of the users with arm cpu reported that ftp vfs does not work and suggested me to change the line in ftpfs.c:
from
hints.ai_flags = AI_ADDRCONFIG;
to
hints.ai_flags = 1;
ftp vfs with this change works for him.
Here is his post (in Russian)
http://www.synology-forum.ru/index.php?showtopic=38&view=findpost&p=8826
See also some posts after this one.

PS I've used sources 4.7.4 from https://www.midnight-commander.org/downloads

Change History

comment:1 follow-up: ↓ 2 Changed 6 years ago by andrew_b

Well, such bug was fixed in #1726. Does that fix not work for you?

As I can see posts in synology-forum, the problem might be due to broken IPv6 support (#121). If yes, you should wait for 4.7.5 release or use current master branch snapshot from git repo.

comment:2 in reply to: ↑ 1 Changed 6 years ago by andrew_b

Replying to andrew_b:

Well, such bug was fixed in #1726. Does that fix not work for you?

Ah, sorry. MC is buidable but not workable. Thus, 0x0020 value for AI_ADDRCONFIG doesn't good for you.

comment:3 Changed 6 years ago by zyxmon

As for me - I have no problems with synology with Marvel Kirkwood 88F6281 (ARM) CPU. FTP VFS works for me out of the box.
But vvivanov that has Marvell mv5281 ARM Processor based synology has the problem.
Ipv6 is not enabled on his NAS. But he also has a newer firmware.
I'll wait for 4.7.5 and will make packages for all synology NAS (different cpu's) when it is released (now I crosscompile mc for 4 out of 5 platforms - http://forum.zyxmon.org/topic50-nas-synology-midnight-commander-i-russkii-yazyk.html )

As for the new firware that has Ipv6 support and a lot of new features I still wait for a better version with less bugs. May be a new firmware is the reason.

comment:4 Changed 6 years ago by andrew_b

Are there any news?

comment:5 follow-up: ↓ 6 Changed 6 years ago by zyxmon

The bug on Marvell mv5281 platform and mc 4.7.4 was confirmed by 2 users. Both users confirm that a patch in ftpfs.c fixes the problem.
I have crosscompiled 4.7.5 for all Synology platforms a couple of days ago.
I have no problems with ftp on 88F6281 (ARM) CPU.
I have no news/reports from mv5281 users yet.

comment:6 in reply to: ↑ 5 Changed 6 years ago by angel_il

Replying to zyxmon:

I have crosscompiled 4.7.5 for all Synology platforms a couple of days ago.
I have no problems with ftp on 88F6281 (ARM) CPU.
I have no news/reports from mv5281 users yet.

so, can we close this ticket?

comment:7 Changed 6 years ago by zyxmon

With my Synology NAS (88F6281 ARM CPU) this patch was not needed in mc 4.7.4.
I have contacted (PM) one of the users who had this problem and has mv5281 platform. Lets wait for his answer if the problem is fixed.

comment:8 Changed 6 years ago by zyxmon

The patch is still needed for 4.7.5 as reported here
http://www.synology-forum.ru/index.php?showtopic=38&view=findpost&p=10689
(in Russian)

comment:9 Changed 6 years ago by andrew_b

  • Milestone changed from 4.7 to 4.8

Related to #2476.

comment:10 Changed 6 years ago by andrew_b

  • Summary changed from Problems with ftp vfs to Problems with ftp on NAS synology devices based on Marvell mv5281 ARM processor

comment:11 Changed 6 years ago by zaytsev

  • Status changed from new to accepted
  • Owner set to zaytsev
  • Version changed from 4.7.4 to master

comment:12 Changed 6 years ago by zaytsev

  • Keywords NetBSD ftpfs added
  • Cc cheusov added

comment:13 follow-up: ↓ 14 Changed 6 years ago by zaytsev

  • Keywords stable-candidate added

Branch: 2401_ai_addrconfig_optional (parent: master)
Changeset: a290a0b557922c0969dddb42af904e1fe659d56a

Please review, test and vote!

comment:14 in reply to: ↑ 13 Changed 6 years ago by cheusov

Replying to zaytsev:

Branch: 2401_ai_addrconfig_optional (parent: master)
Changeset: a290a0b557922c0969dddb42af904e1fe659d56a

Please review, test and vote!

If I understand your commit message correctly, ticket 2476 was not understood
correctly. NetBSD didn't reject declared but not unimplemented flag AI_ADDRCONFIG.
It just rejected garbage given on input.
AI_ADDRCONFIG macros is not declared anywhere in NetBSD-5.1.
Nor 0x20 constant is a correct bit flag in it.

So, it's unclear for me what's the reason to call getaddrinfo twice, especially
if the second invocation is under e == EAI_BADFLAGS condition.
Is these two calls for uClibc-based Linuxes?

comment:15 follow-up: ↓ 17 Changed 6 years ago by zaytsev

No, you didn't understand it correctly. This macro is *required* to be defined on any POSIX compliant system. Whether to implement its support in getaddrinfo() or just ignore it is optional, as is IPv6 support.

It is true, however, that it is not defined in NetBSD. It's up to NetBSD developers to fix this.

If it is defined, however, it is perfectly possible that the system does not implement it and returns EAI_BADFLAGS error code instead. In this case, one has to retry the lookup without AI_ADDRCONFIG.

comment:16 Changed 6 years ago by zaytsev

P.S. In the case if it is not defined, even if this is considered to be a breakage, it is obvious that the system does not implement it, so it is not worth trying to use it anyway. This is what my patch does.

comment:17 in reply to: ↑ 15 Changed 6 years ago by cheusov

If it is defined, however, it is perfectly possible that the system does not implement it and returns EAI_BADFLAGS error code instead. In this case, one has to retry the lookup without AI_ADDRCONFIG.

Have you ever seen such platform? It is unlikely that someone will declare AI_ADDRCONFIG to a nonzero value and explicitely return EAI_BADFLAGS.
In my view

#ifndef AI_ADDRCONFIG
#define AI_ADDRCONFIG 0
#endif

whould be sufficient.

comment:18 Changed 6 years ago by zaytsev

This would be any platform that does support newer POSIX, but not IPv6. I don't have a specific example at hand, but this is the way APR folks do it, so I see no reason not to follow their lead. I think they do have an idea about portability.

Who would have expected NetBSD does not define these macros? And yet it doesn't. Leave it this way and this ticket will be reopened by some WEIRDIX user later.

comment:19 Changed 6 years ago by slavazanko

  • Votes for changeset set to slavazanko

comment:20 Changed 5 years ago by andrew_b

  • Votes for changeset changed from slavazanko to slavazanko andrew_b
  • severity changed from no branch to approved
  • Milestone changed from 4.8 to 4.8.0-pre1

comment:21 Changed 5 years ago by zaytsev

Thanks guys, will merge ASAP, a bit overwhelmed at the moment =(

comment:23 Changed 5 years ago by zaytsev

  • Votes for changeset changed from slavazanko andrew_b to committed-master
  • severity changed from approved to merged

Committed to master. Merge changeset: 14a82ae0ec246771ee23a9cd2726262e29c0d7bc.

comment:24 Changed 5 years ago by zaytsev

Cherry-picked to 4.7.5-stable, changeset: 254eeb59c1c8bc595c62a5342ea6a3c5fa3e7650.

comment:25 Changed 5 years ago by zaytsev

Cherry-picked to 4.7.0-stable, changeset: 1f50cb89d08f3b91449e4e630a1846b5031f45e5.

comment:26 Changed 5 years ago by zaytsev

  • Status changed from accepted to testing
  • Keywords stable-candidate removed
  • Votes for changeset changed from committed-master to committed-master committed-stable
  • Resolution set to fixed

Updated the NEWS.

comment:27 Changed 5 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.