Ticket #1818 (closed defect: fixed)

Opened 10 years ago

Last modified 9 years ago

Refactoring no-vfs

Reported by: metux Owned by: andrew_b
Priority: major Milestone: 4.7.4
Component: mc-vfs Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: Votes for changeset: committed-master

Description (last modified by andrew_b) (diff)

Building w/o VFS seems quite a bit broken now.
Refactoring it, eg. moving stuff from src/vfsdummy.h into vfs/vfs.h and get rid of some #ifdef VFS

Change History

comment:1 Changed 10 years ago by andrew_b

  • Component changed from mc-core to mc-vfs

comment:2 Changed 10 years ago by metux

  • Keywords vfs no-vfs added
  • Status changed from new to accepted
  • severity changed from no branch to on review
  • Owner set to metux
  • Description modified (diff)

comment:3 Changed 10 years ago by andrew_b

  • severity changed from on review to on rework

vfs/vfs.h

223	    p = strdup (p); 

254	#endif                          /* MC_VFSDUMMY_H */

Please fix that.

And use g_strdup() instead of strdup() and g_snprintf() instead of snprintf().

comment:4 Changed 10 years ago by metux

In which way is g_strdup()/g_snprint() better than strdup()/snprintf(), despite being slower ?

comment:5 Changed 10 years ago by andrew_b

Actually, in most cases (99%) that glib functions are used in MC.

comment:6 follow-up: ↓ 8 Changed 10 years ago by metux

That doesnt change the fact, that these two functions are nothing but slow wrappers, without any technical benetif it that case.

Just look at the source:
g_snprintf() calls g_vsnprintf() calls vsnprintf()

Calling snprintf() is the direct way. And a clever compiler can
optimize much more (eg. inlining statically predictable cases).

comment:7 Changed 10 years ago by andrew_b

Just look at glibc source. You'll see the same way.

comment:8 in reply to: ↑ 6 Changed 10 years ago by andrew_b

Replying to metux:

That doesnt change the fact, that these two functions are nothing but slow wrappers,

OK, let's compare:

#include <stdio.h>
#include <sys/types.h>

int
main (int argc, char *argv[])
{
 size_t i, j;
 char buf[32];

 for (i = 0; i < 10000; i++)
     for (j = 0; j < 10000; j++)
         snprintf (buf, sizeof (buf), "%ll", i + j);

 return 0;
}

/* Result:
   $ time ./test-snprintf
   9.50user 0.00system 0:09.51elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
   0inputs+0outputs (0major+114minor)pagefaults 0swaps
*/
#include <sys/types.h>
#include <glib.h>

int
main (int argc, char *argv[])
{
 size_t i, j;
 char buf[32];

 for (i = 0; i < 10000; i++)
     for (j = 0; j < 10000; j++)
         g_snprintf (buf, sizeof (buf), "%ll", i + j);

 return 0;
}

/* Result:
   $ time ./test-gsnprintf
   10.14user 0.00system 0:10.15elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
   0inputs+0outputs (0major+137minor)pagefaults 0swaps
*/

Tested on Intel(R) Core(TM)2 Duo CPU E6850 @ 3.00GHz

Yes, slower by 0.6 s for 100,000,000 iterations. Is it too much for you?

comment:9 follow-up: ↓ 10 Changed 10 years ago by andrew_b

  • Description modified (diff)

Enrico, you stubbornly replace glib functions. I don't thing that you will give any votes for your changes.

comment:10 in reply to: ↑ 9 Changed 10 years ago by andrew_b

Replying to andrew_b:

I don't thing that you will give any votes for your changes.

s/give/get
of course. Sorry...

comment:11 Changed 10 years ago by andrew_b

Sorry, too many typos. My morning is not very well...

I don't thing that you will give any votes for your changes.

I don't think that you will get any votes for your changes.

comment:12 Changed 10 years ago by slavazanko

  • severity changed from on rework to on review

Well... In branch was huge work. It's great. But I have some proposal:

a34369f96646c35bc01ee09fa75ae44be3478bd1::

configure.ac: Added check for empty value of 'enable_mcserver' variable for more proper summary output.

d31b2e86191ee5ea61dfe4e4b55962a1b6a698f5::

Remove support of mvfs. Need to start anoter task for adding them into project. Just keep in mind this changeset if need revert in future. At present time we don't have mvfs support. See #1829 for more details

e4f78f2a928ff5d9bad8b93e8bdf00ed2a9e0bf2::

Replaced free() via g_free() function for one-style codetyping. Please, use g_* functions if this possible.

5c0af26f3bcbd8da915859307fe3323522ae2e84::

Added author of 'vfs/netutil.c' file (but not author of functions in this file :) )

Also, no need to rebase branch at any time of change 'master' branch. In ideal case this branch should be rebased before merge in 'master' branch. Please, don't often rebase branches.

Review, please.

comment:13 Changed 10 years ago by metux

As I'm still reworking w/ lots of rebasing, I've almost overwritten your changes by accident ;-O

So I've moved to the 1818_refactoring_no_vfs_METUX branch. Don't touch it - it will be overwritten ;-P

I'll let you know when I'm done and everything runs through the build matrix.

comment:14 Changed 10 years ago by slavazanko

  • severity changed from on review to on rework

:) okay, awaiting...

comment:15 Changed 10 years ago by metux

Okay, finished for now and testbuilds succeed.

For additions better chose a different branch name :)

comment:16 Changed 9 years ago by metux

  • Milestone changed from 4.7 to 4.7.0

comment:17 Changed 9 years ago by metux

  • Description modified (diff)
  • severity changed from on rework to on review

rebased to current master, running through testfarm.

comment:18 follow-up: ↓ 20 Changed 9 years ago by slavazanko

Present two branches: 1818_refactoring_no_vfs and 1818_refactoring_no_vfs_METUX
What actual? What reason of creating of second branch?

comment:19 Changed 9 years ago by angel_il

  • Milestone changed from 4.7.0 to 4.7

comment:20 in reply to: ↑ 18 Changed 9 years ago by metux

Replying to slavazanko:

Present two branches: 1818_refactoring_no_vfs and 1818_refactoring_no_vfs_METUX
What actual? What reason of creating of second branch?

I'm entirely working in 1818_refactoring_no_vfs_METUX, leaving the other branch (w/ your changes) untouched.

comment:21 Changed 9 years ago by slavazanko

  • severity changed from on review to on rework

branch 1818_refactoring_no_vfs_METUX looks like incorrectly rebased.
For example, branch contain these strange commits:

Please, reorganize branch with proper set of commits.

comment:22 Changed 9 years ago by metux

  • Blocked By 1897 added

It's not ready for merge yet, since it's based on #1897.

But please keep an eye on this branch :)

comment:23 Changed 9 years ago by metux

  • severity changed from on rework to on review

Rebased to current master. Please review.

comment:24 Changed 9 years ago by andrew_b

  • severity changed from on review to on rework

Branch 1818_refactoring_no_vfs_METUX contains a lot of unrelated commits:

ignoring errors on chdir() call (to make the compiler happy ;-o)
ignore fscanf() result in edit_delete_macro() (make compiler happy ;-o)
ignoring IO errors in background_attention() (make compiler happy ;-o)
and so on.

Please clean your branch. It must not contain commits of #1897.

comment:25 follow-up: ↓ 26 Changed 9 years ago by metux

No, that's just because it's based on the 1897_libc_return_values
branch.

Please review / approve that one first.

comment:26 in reply to: ↑ 25 Changed 9 years ago by andrew_b

Replying to metux:

No, that's just because it's based on the 1897_libc_return_values
branch.

These issues are independent. Please rebase 1818 branch to master.

And 1818_refactoring_no_vfs_METUX branch is not bildable:

src/Makefile.am:60: USE_SAMBA_FS does not appear in AM_CONDITIONAL
automake failed to generate Makefile.in

comment:27 Changed 9 years ago by andrew_b

  • Owner changed from metux to andrew_b
  • severity changed from on rework to no branch
  • Blocked By 1897 removed

comment:28 Changed 9 years ago by andrew_b

  • Description modified (diff)
  • Version changed from 4.7.0-pre4 to master
  • severity changed from no branch to on review
  • Milestone changed from 4.7 to 4.7.4

Created new branch 1818_refactoring_no_vfs.
Initial changeset:43a288cbf68ae5f8090519f974085b4bd69ccd4a

comment:29 Changed 9 years ago by slavazanko

  • Votes for changeset set to slavazanko

comment:30 Changed 9 years ago by angel_il

  • Votes for changeset changed from slavazanko to slavazanko angel_il
  • severity changed from on review to approved

comment:31 Changed 9 years ago by andrew_b

  • Keywords vfs no-vfs removed
  • Status changed from accepted to testing
  • Votes for changeset changed from slavazanko angel_il to committed-master
  • Resolution set to fixed
  • severity changed from approved to merged

Merged to master.
changeset:43b9c754b4fa78ca3cf82aa90fe1bba39d440c3f

git log --pretty=oneline e04291c..43b9c75

comment:32 Changed 9 years ago by andrew_b

  • Status changed from testing to closed

comment:33 Changed 9 years ago by andrew_b

  • Status changed from closed to reopened
  • Votes for changeset committed-master deleted
  • Resolution fixed deleted
  • severity changed from merged to no branch

configure of samba runs if --enable-vfs-smb=no (or even --disable-vfs) option is set.

comment:34 Changed 9 years ago by andrew_b

Created 1818_vfs_smb branch. Parent branch is master.
Initial changeset:59ef1938b330a3fb5fef2400fd32477e432bb9d8

comment:35 Changed 9 years ago by andrew_b

  • severity changed from no branch to on review

comment:36 Changed 9 years ago by slavazanko

  • Votes for changeset set to slavazanko

comment:37 Changed 9 years ago by angel_il

  • Votes for changeset changed from slavazanko to slavazanko angel_il
  • severity changed from on review to approved

comment:38 Changed 9 years ago by andrew_b

  • Status changed from reopened to closed
  • Votes for changeset changed from slavazanko angel_il to committed-master
  • Resolution set to fixed
  • severity changed from approved to merged

Merged to master.
changeset:bae864990ed66eef4cfee8272cfd91461bbe0402

git log --pretty=oneline eb7a91c..bae8649
Note: See TracTickets for help on using tickets.