cp(1) could use sendfile

Review Request #1311 — Created Dec. 2, 2018 and updated

Information
wiedi
illumos-gate
Reviewers
general 
Change to libcmdutils so that cp(1) uses the sendfile(3ext) interface.

This avoids copying the data between kernel and userspace.
My testing so far includes using this cp on trees with various file

sizes. I've tested that checksums still match. I've tested error handling

is unchanged when reaching the fs quota. I also booted into a system with this change.

I have not tested this with NFS because I currently don't have such a setup.
  • 2
  • 0
  • 0
  • 1
  • 3
Description From Last Updated
By using sendfile(), you've changed the state in which fi will be left. sendfile() does not modify the file pointer ... citrus citrus
You still need to free srcbuf and targbuf here. I'd actually be tempted to add an err: label near the ... citrus citrus
citrus
citrus 
  1. libcmdutils:writefile() is also used by mv in some cases (cross-filesystem move for example) so that should be included in the tests.
  2. Not important, but I'd prefer this be a global variable to allow easier manipulation with mdb.
  3. Show all issues 
    By using sendfile(), you've changed the state in which fi will be left. sendfile() does not modify the file pointer of the input file unlike the previous code.
    

    Do any consumers of writefile() care? I suspect not but you should check.
  4. Why continue here if a signal is received?
  5. Show all issues 
    You still need to free srcbuf and targbuf here.
    
I'd actually be tempted to add an err: label near the bottom of the function and consolidate 
    

        (void) close(fi);
    (void) close(fo);
    if (S_ISREG(s2p->st_mode))
        (void) unlink(targbuf);
    if (srcbuf != NULL)
        free(srcbuf);
    if (targbuf != NULL)
        free(targbuf);
    return (1);

    

    which appears three times in this function
  6. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 





 

  

   




andy_js




   

   







  

 



 

  


   

   

    
 
andy_js


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    You should be able to send the entire file with a single call to sendfile().  Ditch the loop.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Without the loop a user would not be able to cancel a cp process sending a very large file. It will appear hanging until everything has been written.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Interesting.  I sort of assumed that SIGINT would cause it to return EINTR.
      
 
      

      3. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 









     

    

   

  

  

   
   Loading...