5980 in-kernel inet_ntop should format IPv4 addresses like userland one

Review Request #622 — Created July 7, 2017 and submitted

Information
yuripv
illumos-gate
master
5980
185011c...
Reviewers
general 
This changes the format inet_ntop() uses for IPv4 addresses to be the 

same as userland one (which makes more sense).


The list of current kernel's inet_ntop() consumers with comments:


https://gist.github.com/yuripv/cbf201e7450eab867b510577cdb8188c


The only consumer that requires attention is 

usr/src/uts/common/io/scsi/adapters/iscsi/persistent.c as it uses the IP 

addresses as keys into persistent storage, so I've added a wrapper there 

keeping old inet_ntop() behaviour - adding the logic which would rewrite 

the nvlist names for such entries is just not worth it IMO.


Everything else is using inet_ntop() to print log messages, or was 

already using a wrapper to fix the format (like SMB and iscsit).
Checked that iscsiadm can delete/update the entries in persistent storage that were created in previous BE.
  • 0
  • 0
  • 0
  • 4
  • 4
Description From Last Updated
igork
igork 
  1. Ship It!
  2. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




vgusev




   

   







  

 



 

  


   

   

    
 
vgusev


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/common/smbsrv/smb_inet.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    It looks like this function is redundant since now. Doesn't it?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I'll let Gordon decide about this one as it's used both in userland and kernel.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Thanks, it could be as for iscsit_v4_ntop.  Other things look good.
      
 
      

      3. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/common/smbsrv/smb_inet.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Why #if/#else is required after fixed inet_ntop() ?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Same answer as above, it's both for userland and kernel, so at least needs different types of addrlen argument.
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/uts/common/inet/ip/inet_ntop.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Why @addrlen type is 'int' and not socklen_t ?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Same reason it's size_t in userland - legacy - and I'm not "fixing" in this change.
      
 
      

      2. 


 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 





 

  

   




vgusev




   

   







  

 



 

  


   

   

    
 
vgusev


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/uts/common/io/idm/idm_so.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Why did you remove this check? Without it snprintf() below can silently truncate result.
    
With this check the function will return 'bogus_ip' if size is too small.
    
So this patch changes function behaviour whereas the patch should be just refactoring as I understand. Or you wanted to change this behaviour  intentionally ?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I didn't? I just reformated this to be properly indented and removed stupid checks for inet_ntop() return value being NULL, please re-read.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      You are right.
      
 
      

      3. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




vgusev




   

   







  

 



 

  


   

   

    
 
vgusev


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




yuripv




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...