Hi Jim, Thanks for your prompt response and kindly suggestion! I totally agree with your review comments, I will post next round patches according to that soon.
Regards, -Jeff Jim Meyering wrote: > jeff.liu wrote: >> Hi Jim and All, >> >> Do you have any comments for the current implementation? > > Hi Jeff, > > Thanks for the reminder. > I've just gone back and looked at those patches: > > http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/20534/focus=21008 > > There are some superficial problems. > > First, whenever you move code from one place to another, > the commit that performs the move should be careful to > induce no other change. In this case, the change should > remove code from copy.c and create the new .c file with > code that is essentially identical. You'll have to remove > a "static" attribute on the primary function(s), and add > #include directives in the new file, but those are inevitable. > Also, in copy.c, you will remove the function body > and associated #include directives, adding an #include > of the new .h file. Of course, this change must also update > src/Makefile.am, and the result should pass "make distcheck". > > But perhaps you require new functions like init_extent_scan > in order to use the abstracted function properly. > In that case, your first commit would add the new functions > in copy.c and make use of them there. Then you would move > all of the functions to their new file in the next commit, > making no semantic change. > > Note however, that this copying code is intended to be usable in a > multi-threaded application, and thus must avoid using internal static > state. Your patch adds a few new static variables, each of which > would cause trouble in such an application: > > +static size_t current_scanned_extents_count = 0; > + static uint64_t next_map_start = 0; > + static size_t i = 0; > > While you're rearranging your patch along the lines above, > please eliminate those static variables, too. > > Also, this new function should be adjusted: > > +/* Write zeros as holes to the destination file. */ > +static bool > +fill_with_holes_ok (int dest_fd, const char *dst_name, > + char *buf, size_t buf_size, > + uint64_t holes_len) > > Its signature is unnecessarily complicated for a function > that does nothing more than write N zero bytes to a file descriptor. > Also, the function name is misleading (as is its holes_len parameter), > since it certainly does not create holes. > > Hmm... now I'm suspicious: could the second use of your fill_with_holes_ok > write from an uninitialized "buf"? It appears that is possible, > but I confess not to have applied the patch. > > What do you think of this signature, > > static bool > write_zeros (int fd, uint64_t n_bytes) > > That would require a buffer full of zeros, preferably of optimal size. > It could use a body like this, > > { > static char zero[32 * 1024]; > while (n_bytes) > { > uint64_t n = MIN (sizeof zero, n_bytes); > if (full_write (fd, zero, n)) != n) > return false; > n_bytes -= n; > } > return true; > } > > or even calloc an IO_BUFSIZE-byte buffer on the first call > and use that. Yes, using calloc appears better, since this code > will end up being used relatively infrequently. Or perhaps better > still, do use calloc, but if the allocation fails, fall back on > using a smaller static buffer, of size say 1024 bytes. > > Of course, simplifying the function means each caller > will have to diagnose failure, but imho, that's preferable > in this case. > > Jim > > >
