From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 15:46:11
This patchset expands the KVP (Key Value Pair) functionality to
implement the mechanism to GET/SET IP addresses in the guest. This
functionality is used in Windows Server 2012 to implement VM
replication functionality. The way IP configuration information
is managed is distro specific. Based on the feedback I have gotten
from Olaf, Greg, Steve, Ben and Mairus, I have chosen to seperate
distro specific code from this patch-set. Most of the GET operation
can be implemented in a way that is completely distro independent and
I have implemented that as such and is included in this patch-set.
Some of the attributes that can only be fetched in a distro
dependent way as well the mechanism for configuring an interface
(the SET operation) that is clearly distro specific is to be
implemented via external scripts that will be invoked via the KVP
code. We define here the interface to these scripts.
Adding support for IP injection resulted in some changes to the
protocol between the user level daemon and the kernel driver.
These changes have been implemented in way that would retain
compatibility with older daemons. I would like to thank Olaf and
Greg for pointing out the compatibility issue.
K. Y. Srinivasan (17):
Drivers: hv: vmbus: Use the standard format string to format GUIDs
Drivers: hv: Add KVP definitions for IP address injection
Drivers: hv: kvp: Cleanup error handling in KVP
Drivers: hv: kvp: Support the new IP injection messages
Tools: hv: Prepare to expand kvp_get_ip_address() functionality
Tools: hv: Further refactor kvp_get_ip_address()
Tools: hv: Gather address family information
Tools: hv: Gather subnet information
Tools: hv: Represent the ipv6 mask using CIDR notation
Tools: hv: Gather ipv[4,6] gateway information
Tools: hv: Gather DNS information
Tools: hv: Gather DHCP information
Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO
Tools: hv: Rename the function kvp_get_ip_address()
Tools: hv: Implement the KVP verb - KVP_OP_GET_IP_INFO
Tools: hv: Get rid of some unused variables
Tools: hv: Correctly type string variables
drivers/hv/hv_kvp.c | 251 +++++++++++--
drivers/hv/hv_util.c | 4 +-
drivers/hv/vmbus_drv.c | 38 +--
include/linux/hyperv.h | 88 ++++-
tools/hv/hv_kvp_daemon.c | 943 +++++++++++++++++++++++++++++++++++++++++-----
5 files changed, 1160 insertions(+), 164 deletions(-)
--
1.7.4.1
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 15:46:39
Format GUIDS as per MSFT standard. This makes interacting with MSFT
tool stack easier.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/vmbus_drv.c | 38 ++------------------------------------
1 files changed, 2 insertions(+), 36 deletions(-)
@@ -122,12 +122,53 @@#define REG_U32 4#define REG_U64 8+/*+*AswelookatexpandingtheKVPfunctionalitytoinclude+*IPinjectionfunctionality,weneedtomaintainbinary+*compatibilitywitholderdaemons.+*+*TheKVPopcodesaredefinedbythehostanditwasunfortunate+*thatIchosetotreattheregistrationoperationaspartofthe+*KVPoperationsdefinedbythehost.+*Hereisthelevelofcompatibility+*(betweentheuserleveldaemonandthekernelKVPdriver)thatwe+*willimplement:+*+*Anolderdaemonwillalwaysbesupportedonanewerdriver.+*Agivenuserleveldaemonwillrequireaminimalversionofthe+*kerneldriver.+*Ifwecannothandletheversiondifferences,wewillfailgracefully+*(thiscanhappenwhenwehaveauserleveldaemonthatismore+*advancedthantheKVPdriver.+*+*Wewillusevaluesusedinthishandshakefordeterminingifwehave+*workableuserleveldaemonandthekerneldriver.Webeginbytakingthe+*registrationopcodeoutoftheKVPopcodenamespace.Wewillhowever,+*maintaincompatibilitywiththeexistinguser-leveldaemoncode.+*/++/*+*DaemoncodenotsupportingIPinjection(legacydaemon).+*/++#define KVP_OP_REGISTER 4++/*+*DaemoncodesupportingIPinjection.+*TheKVPopcodefieldisusedtocommunicatethe+*registrationinformation;sodefineanamespacethat+*willbedistinctfromthehostdefinedKVPopcode.+*/++#define KVP_OP_REGISTER1 100+enumhv_kvp_exchg_op{KVP_OP_GET=0,KVP_OP_SET,KVP_OP_DELETE,KVP_OP_ENUMERATE,-KVP_OP_REGISTER,+KVP_OP_GET_IP_INFO,+KVP_OP_SET_IP_INFO,KVP_OP_COUNT/* Number of operations, must be last. */};
@@ -140,6 +181,26 @@ enum hv_kvp_exchg_pool {KVP_POOL_COUNT/* Number of pools, must be last. */};+#define ADDR_FAMILY_NONE 0x00+#define ADDR_FAMILY_IPV4 0x01+#define ADDR_FAMILY_IPV6 0x02++#define MAX_ADAPTER_ID_SIZE 128+#define MAX_IP_ADDR_SIZE 1024+#define MAX_GATEWAY_SIZE 512+++structhv_kvp_ipaddr_value{+__u16adapter_id[MAX_ADAPTER_ID_SIZE];+__u8addr_family;+__u8dhcp_enabled;+__u16ip_addr[MAX_IP_ADDR_SIZE];+__u16sub_net[MAX_IP_ADDR_SIZE];+__u16gate_way[MAX_GATEWAY_SIZE];+__u16dns_addr[MAX_IP_ADDR_SIZE];+}__attribute__((packed));++structhv_kvp_hdr{__u8operation;__u8pool;
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 15:47:07
Correctly type character strings. I would like to thank Ben Hutchings
for pointing out the issue (ben@decadent.org.uk).
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 15:47:09
Get rid of unused variables. I would like to thank Ben Hutchings
for pointing out the issue (ben@decadent.org.uk).
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
@@ -322,7 +322,6 @@ static int kvp_key_add_or_modify(int pool, __u8 *key, int key_size, __u8 *value,intvalue_size){inti;-intj,k;intnum_records;structkvp_record*record;intnum_blocks;
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 15:47:14
Now, gather address family information for the specified interface.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 15:48:34
In preparation for making kvp_get_ip_address() more generic, factor out
the code for handling IP addresses.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 94 ++++++++++++++++++++-------------------------
1 files changed, 42 insertions(+), 52 deletions(-)
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 15:49:07
In preparation to implementing IP injection, cleanup the way we propagate
and handle errors both in the driver as well as in the user level daemon.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/hv_kvp.c | 112 +++++++++++++++++++++++++++++++++++++---------
include/linux/hyperv.h | 17 +++++---
tools/hv/hv_kvp_daemon.c | 70 +++++++++++++++-------------
3 files changed, 138 insertions(+), 61 deletions(-)
@@ -394,7 +395,7 @@ static int kvp_get_value(int pool, __u8 *key, int key_size, __u8 *value,return1;}-staticvoidkvp_pool_enumerate(intpool,intindex,__u8*key,intkey_size,+staticintkvp_pool_enumerate(intpool,intindex,__u8*key,intkey_size,__u8*value,intvalue_size){structkvp_record*record;
@@ -406,16 +407,12 @@ static void kvp_pool_enumerate(int pool, int index, __u8 *key, int key_size,record=kvp_file_info[pool].records;if(index>=kvp_file_info[pool].num_records){-/*-*Thisisaninvalidindex;terminateenumeration;-*-aNULLvaluewilldothetrick.-*/-strcpy(value,"");-return;+return1;}memcpy(key,record[index].key,key_size);memcpy(value,record[index].value,value_size);+return0;}
@@ -646,6 +643,8 @@ int main(void)char*p;char*key_value;char*key_name;+intop;+intpool;daemon(1,0);openlog("KVP",0,LOG_USER);
@@ -687,7 +686,7 @@ int main(void)message->id.val=CN_KVP_VAL;hv_msg=(structhv_kvp_msg*)message->data;-hv_msg->kvp_hdr.operation=KVP_OP_REGISTER;+hv_msg->kvp_hdr.operation=KVP_OP_REGISTER1;message->ack=0;message->len=sizeof(structhv_kvp_msg);
@@ -721,12 +720,21 @@ int main(void)incoming_cn_msg=(structcn_msg*)NLMSG_DATA(incoming_msg);hv_msg=(structhv_kvp_msg*)incoming_cn_msg->data;-switch(hv_msg->kvp_hdr.operation){-caseKVP_OP_REGISTER:+/*+*WewillusetheKVPheaderinformationtopassback+*theerrorfromthisdaemon.So,firstcopythestate+*andsettheerrorcodetosuccess.+*/+op=hv_msg->kvp_hdr.operation;+pool=hv_msg->kvp_hdr.pool;+*((int*)(&hv_msg->kvp_hdr.operation))=HV_S_OK;++if((in_hand_shake)&&(op==KVP_OP_REGISTER1)){/**Driverisregisteringwithus;stashawaytheversion*information.*/+in_hand_shake=0;p=(char*)hv_msg->body.kvp_register.version;lic_version=malloc(strlen(p)+1);if(lic_version){
@@ -737,44 +745,42 @@ int main(void)syslog(LOG_ERR,"malloc failed");}continue;+}-/*-*Thecurrentprotocolwiththekernelcomponentusesa-*NULLkeynametopassanerrorcondition.-*FortheSET,GETandDELETEoperations,-*usetheexistingprotocoltopassbackerror.-*/-+switch(op){caseKVP_OP_SET:-if(kvp_key_add_or_modify(hv_msg->kvp_hdr.pool,+if(kvp_key_add_or_modify(pool,hv_msg->body.kvp_set.data.key,hv_msg->body.kvp_set.data.key_size,hv_msg->body.kvp_set.data.value,hv_msg->body.kvp_set.data.value_size))-strcpy(hv_msg->body.kvp_set.data.key,"");+*((int*)(&hv_msg->kvp_hdr.operation))=+HV_S_CONT;break;caseKVP_OP_GET:-if(kvp_get_value(hv_msg->kvp_hdr.pool,+if(kvp_get_value(pool,hv_msg->body.kvp_set.data.key,hv_msg->body.kvp_set.data.key_size,hv_msg->body.kvp_set.data.value,hv_msg->body.kvp_set.data.value_size))-strcpy(hv_msg->body.kvp_set.data.key,"");+*((int*)(&hv_msg->kvp_hdr.operation))=+HV_S_CONT;break;caseKVP_OP_DELETE:-if(kvp_key_delete(hv_msg->kvp_hdr.pool,+if(kvp_key_delete(pool,hv_msg->body.kvp_delete.key,hv_msg->body.kvp_delete.key_size))-strcpy(hv_msg->body.kvp_delete.key,"");+*((int*)(&hv_msg->kvp_hdr.operation))=+HV_S_CONT;break;default:break;}-if(hv_msg->kvp_hdr.operation!=KVP_OP_ENUMERATE)+if(op!=KVP_OP_ENUMERATE)gotokvp_done;/*
@@ -782,13 +788,15 @@ int main(void)*boththekeyandthevalue;ifnotreadfromthe*appropriatepool.*/-if(hv_msg->kvp_hdr.pool!=KVP_POOL_AUTO){-kvp_pool_enumerate(hv_msg->kvp_hdr.pool,+if(pool!=KVP_POOL_AUTO){+if(kvp_pool_enumerate(pool,hv_msg->body.kvp_enum_data.index,hv_msg->body.kvp_enum_data.data.key,HV_KVP_EXCHANGE_MAX_KEY_SIZE,hv_msg->body.kvp_enum_data.data.value,-HV_KVP_EXCHANGE_MAX_VALUE_SIZE);+HV_KVP_EXCHANGE_MAX_VALUE_SIZE))+*((int*)(&hv_msg->kvp_hdr.operation))=+HV_S_CONT;gotokvp_done;}
@@ -841,11 +849,7 @@ int main(void)strcpy(key_name,"ProcessorArchitecture");break;default:-strcpy(key_value,"Unknown Key");-/*-*Weuseanullkeynametoterminateenumeration.-*/-strcpy(key_name,"");+*((int*)(&hv_msg->kvp_hdr.operation))=HV_S_CONT;break;}/*
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 15:49:09
Implement support for the new IP injection messages in the driver code.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/hv_kvp.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 137 insertions(+), 6 deletions(-)
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 15:49:10
kvp_get_ip_address() implemented the functionality to retrieve IP address info.
Make this function more generic so that we could retrieve additional
per-interface information.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 129 ++++++++++++++++++++++++++++++----------------
1 files changed, 84 insertions(+), 45 deletions(-)
On Tue, Jul 24, 2012 at 09:01:12AM -0700, K. Y. Srinivasan wrote:
This patchset expands the KVP (Key Value Pair) functionality to
implement the mechanism to GET/SET IP addresses in the guest. This
functionality is used in Windows Server 2012 to implement VM
replication functionality. The way IP configuration information
is managed is distro specific. Based on the feedback I have gotten
from Olaf, Greg, Steve, Ben and Mairus, I have chosen to seperate
distro specific code from this patch-set. Most of the GET operation
can be implemented in a way that is completely distro independent and
I have implemented that as such and is included in this patch-set.
Some of the attributes that can only be fetched in a distro
dependent way as well the mechanism for configuring an interface
(the SET operation) that is clearly distro specific is to be
implemented via external scripts that will be invoked via the KVP
code. We define here the interface to these scripts.
Adding support for IP injection resulted in some changes to the
protocol between the user level daemon and the kernel driver.
These changes have been implemented in way that would retain
compatibility with older daemons. I would like to thank Olaf and
Greg for pointing out the compatibility issue.
Due to this being the middle of the merge window, I will not be able to
look at this until after 3.6-rc1 is out.
greg k-h
What's wrong with the hweight32 version we have already in
<include/asm-generic/bitops/const_hweight.h> which you can include by
simply by including <asm-generic/bitops.h>?
--
Regards/Gruss,
Boris.
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 16:17:23
Collect information on dhcp setting for the specified interface.
We invoke an exyernal script to get this information.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 33 +++++++++++++++++++++++++++++++++
1 files changed, 33 insertions(+), 0 deletions(-)
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 16:17:24
Now implement the KVP verb - KVP_OP_GET_IP_INFO. This operation retrieves IP
information for the specified interface.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 94 ++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 91 insertions(+), 3 deletions(-)
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 16:17:25
Implement the KVP verb - KVP_OP_SET_IP_INFO. This operation configures the
specified interface based on the given configuration. Since configuring
an interface is very distro specific, we invoke an external script to
configure the interface.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
include/linux/hyperv.h | 2 +
tools/hv/hv_kvp_daemon.c | 440 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 442 insertions(+), 0 deletions(-)
@@ -800,6 +910,314 @@ getaddr_done:}+staticintexpand_ipv6(char*addr,inttype)+{+intret;+structin6_addrv6_addr;++ret=inet_pton(AF_INET6,addr,&v6_addr);++if(ret!=1){+if(type==NETMASK)+return1;+return0;+}++sprintf(addr,"%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:"+"%02x%02x:%02x%02x:%02x%02x",+(int)v6_addr.s6_addr[0],(int)v6_addr.s6_addr[1],+(int)v6_addr.s6_addr[2],(int)v6_addr.s6_addr[3],+(int)v6_addr.s6_addr[4],(int)v6_addr.s6_addr[5],+(int)v6_addr.s6_addr[6],(int)v6_addr.s6_addr[7],+(int)v6_addr.s6_addr[8],(int)v6_addr.s6_addr[9],+(int)v6_addr.s6_addr[10],(int)v6_addr.s6_addr[11],+(int)v6_addr.s6_addr[12],(int)v6_addr.s6_addr[13],+(int)v6_addr.s6_addr[14],(int)v6_addr.s6_addr[15]);++return1;++}++staticintis_ipv4(char*addr)+{+intret;+structin_addripv4_addr;++ret=inet_pton(AF_INET,addr,&ipv4_addr);++if(ret==1)+return1;+return0;+}++staticintparse_ip_val_buffer(char*in_buf,int*offset,+char*out_buf,intout_len)+{+char*x;+char*start;++/*+*in_bufhassequenceofcharactersthatareseperatedby+*thecharacter';'.Thelastsequencedoesnothavethe+*terminating";"character.+*/+start=in_buf+*offset;++x=strchr(start,';');+if(x)+*x=0;+else+x=start+strlen(start);++if(strlen(start)!=0){+inti=0;+/*+*Getridofleadingspaces.+*/+while(start[i]==' ')+i++;++if((x-start)<=out_len){+strcpy(out_buf,(start+i));+*offset+=(x-start)+1;+return1;+}+}+return0;+}++staticintkvp_write_file(FILE*f,char*s1,char*s2,char*s3)+{+charstr[256];+interror;++memset(str,0,sizeof(str));+strcat(str,s1);+if(s2!=NULL)+strcat(str,s2);+strcat(str,"=");+strcat(str,s3);+strcat(str,"\n");++error=fputs(str,f);+if(error==EOF)+returnHV_E_FAIL;++return0;+}+++staticintprocess_ip_string(FILE*f,char*ip_string,inttype)+{+interror=0;+charaddr[INET6_ADDRSTRLEN];+inti=0;+intj=0;+charstr[256];+charsub_str[10];+intoffset=0;++memset(addr,0,sizeof(addr));++while(parse_ip_val_buffer(ip_string,&offset,addr,+(MAX_IP_ADDR_SIZE*2))){+memset(sub_str,0,sizeof(sub_str));+memset(str,0,sizeof(str));++if(is_ipv4(addr)){+switch(type){+caseIPADDR:+strcat(str,"IPADDR");+break;+caseNETMASK:+strcat(str,"NETMASK");+break;+caseGATEWAY:+strcat(str,"GATEWAY");+break;+caseDNS:+strcat(str,"DNS");+break;+}+if(i!=0){+if(type!=DNS)+sprintf(sub_str,"_%d",i++);+else+sprintf(sub_str,"%d",++i);+}elseif(type==DNS){+sprintf(sub_str,"%d",++i);+}+++}elseif(expand_ipv6(addr,type)){+switch(type){+caseIPADDR:+strcat(str,"IPV6ADDR");+break;+caseNETMASK:+strcat(str,"IPV6NETMASK");+break;+caseGATEWAY:+strcat(str,"IPV6_DEFAULTGW");+break;+caseDNS:+strcat(str,"DNS");+break;+}+if((j!=0)||(type==DNS)){+if(type!=DNS)+sprintf(sub_str,"_%d",j++);+else+sprintf(sub_str,"%d",++i);+}elseif(type==DNS){+sprintf(sub_str,"%d",++i);+}+}else{+returnHV_INVALIDARG;+}++error=kvp_write_file(f,str,sub_str,addr);+if(error)+returnerror;+memset(addr,0,sizeof(addr));+}++return0;+}++staticintkvp_set_ip_info(char*if_name,structhv_kvp_ipaddr_value*new_val)+{+interror=0;+charif_file[50];+FILE*file;+charcmd[512];+char*mac_addr;++/*+*Settheconfigurationforthespecifiedinterfacewith+*theinformationprovided.Sincethereisnostandard+*waytoconfigureaninterface,wewillhaveanexternal+*scriptthatdoesthejobofconfiguringtheinterfaceand+*flushingtheconfiguration.+*+*Theparameterspassedtothisexternalscriptare:+*1.Aconfigurationfilethathasthespecifiedconfiguration.+*+*Wewillembedthenameoftheinterfaceintheconfiguration+*file:ifcfg-ethx(whereethxistheinterfacename).+*+*Hereistheformatoftheipconfigurationfile:+*+*HWADDR=macaddr+*BOOTPROTO=dhcp(dhcpenabledfortheinterface)+*NM_CONTROLLED=no(thisinterfacewillnotbecontrolledbyNM)+*PEERDNS=yes+*IPADDR_x=ipaddr+*NETMASK_x=netmask+*GATEWAY_x=gateway+*DNSx=dns+*+*IPV6addresseswillbetaggedasIPV6ADDR,IPV6gatewaywillbe+*taggedasIPV6_DEFAULTGWandIPV6NETMASKwillbetaggedas+*IPV6NETMASK.+*/++memset(if_file,0,sizeof(if_file));+strcat(if_file,"/var/opt/hyperv/ifcfg-");+strcat(if_file,if_name);++file=fopen(if_file,"w");++if(file==NULL){+syslog(LOG_ERR,"Failed to open config file");+returnHV_E_FAIL;+}++/*+*FirstwriteouttheMACaddress.+*/++mac_addr=kvp_if_name_to_mac(if_name);+if(mac_addr==NULL){+error=HV_E_FAIL;+gotosetval_error;+}++error=kvp_write_file(file,"HWADDR",NULL,mac_addr);+if(error)+gotosetval_error;++error=kvp_write_file(file,"ONBOOT",NULL,"yes");+if(error)+gotosetval_error;++error=kvp_write_file(file,"IPV6INIT",NULL,"yes");+if(error)+gotosetval_error;++error=kvp_write_file(file,"NM_CONTROLLED",NULL,"no");+if(error)+gotosetval_error;++error=kvp_write_file(file,"PEERDNS",NULL,"yes");+if(error)+gotosetval_error;++if(new_val->dhcp_enabled){+error=kvp_write_file(file,"BOOTPROTO",NULL,"dhcp");+if(error)+gotosetval_error;++/*+*Wearedone!.+*/+gotosetval_done;+}++/*+*Writetheconfigurationforipaddress,netmask,gatewayand+*nameservers.+*/++error=process_ip_string(file,(char*)new_val->ip_addr,IPADDR);+if(error)+gotosetval_error;++error=process_ip_string(file,(char*)new_val->sub_net,NETMASK);+if(error)+gotosetval_error;++error=process_ip_string(file,(char*)new_val->gate_way,GATEWAY);+if(error)+gotosetval_error;++error=process_ip_string(file,(char*)new_val->dns_addr,DNS);+if(error)+gotosetval_error;++setval_done:+free(mac_addr);+fclose(file);++/*+*Nowthatwehavepopulatedtheconfigurationfile,+*invoketheexternalscripttodoitsmagic.+*/++memset(cmd,0,sizeof(cmd));+strcat(cmd,"/sbin/hv_set_ifconfig ");+strcat(cmd,if_file);+system(cmd);+return0;++setval_error:+syslog(LOG_ERR,"Failed to write config file");+free(mac_addr);+fclose(file);+returnerror;+}++staticintkvp_get_domain_name(char*buffer,intlength){
@@ -869,6 +1287,8 @@ int main(void)char*key_name;intop;intpool;+char*if_name;+structhv_kvp_ipaddr_value*kvp_ip_val;daemon(1,0);openlog("KVP",0,LOG_USER);
@@ -972,6 +1392,26 @@ int main(void)}switch(op){+caseKVP_OP_SET_IP_INFO:+kvp_ip_val=&hv_msg->body.kvp_ip_val;+if_name=kvp_get_if_name(+(char*)kvp_ip_val->adapter_id);+if(if_name==NULL){+/*+*Wecouldnotmaptheguidtoan+*interfacename;returnerror.+*/+*((int*)(&hv_msg->kvp_hdr.operation))=+HV_GUID_NOTFOUND;+break;+}+error=kvp_set_ip_info(if_name,kvp_ip_val);+if(error)+*((int*)(&hv_msg->kvp_hdr.operation))=error;++free(if_name);+break;+caseKVP_OP_SET:if(kvp_key_add_or_modify(pool,hv_msg->body.kvp_set.data.key,
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 16:17:57
Now gather sub-net information for the specified interface.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 31 +++++++++++++++++++++++++++++--
1 files changed, 29 insertions(+), 2 deletions(-)
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 16:18:22
Rename the function kvp_get_ip_address() to better reflect the functionality
being implemented.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
From: "K. Y. Srinivasan" <kys@microsoft.com> Date: 2012-07-24 16:18:23
Now gather DNS information. This information cannot be gathered in
a distro independent fashion. Invoke an external script (that can be
distro dependent) to gather the DNS information.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 27 +++++++++++++++++++++++++++
1 files changed, 27 insertions(+), 0 deletions(-)
From: KY Srinivasan <kys@microsoft.com> Date: 2012-07-24 16:55:32
-----Original Message-----
From: Borislav Petkov [mailto:bp@alien8.de]
Sent: Tuesday, July 24, 2012 12:01 PM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
apw@canonical.com; netdev@vger.kernel.org; ben@decadent.org.uk
Subject: Re: [PATCH 09/17] Tools: hv: Represent the ipv6 mask using CIDR
notation
On Tue, Jul 24, 2012 at 09:01:33AM -0700, K. Y. Srinivasan wrote:
quoted
Transform ipv6 subnet information to CIDR notation.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 45
What's wrong with the hweight32 version we have already in
<include/asm-generic/bitops/const_hweight.h> which you can include by
simply by including <asm-generic/bitops.h>?
Boris,
This code is a user-level daemon that will be compiled outside of the kernel.
I did not want to include Kernel header files for this one function and deal with
all the dependencies that will have to be dealt with.
Regards,
K. Y
On Tue, Jul 24, 2012 at 04:53:50PM +0000, KY Srinivasan wrote:
This code is a user-level daemon that will be compiled outside of the
kernel. I did not want to include Kernel header files for this one
function and deal with all the dependencies that will have to be dealt
with.
Ah, I missed the tools/ prefix in the patch, sorry.
Well, FWIW, we have hweight32 in <tools/perf/util/hweight.c> too and
there was a patchset preparing a generic kernel tools library where
tools can share functions but the author doesn't have time to dust the
patches off and get it upstream... :-).
I guess having a local function is the easiest for now.
Thanks.
--
Regards/Gruss,
Boris.
This also has the benefit that ip is not called with absolute path, now
that distros move binaries around.
I could have chosen to not specify the full path for the ip command and for that
matter all the external scripts I invoke from the KVP daemon. Do you mind if I
submitted a patch to get rid of the absolute paths in this code.
Stephen's suggestion is clearly simpler (I don't need to invoke awk to filter what
we want). Steve, I could make this change as well as an additional patch.
Regards,
K. Y
From: KY Srinivasan <kys@microsoft.com> Date: 2012-07-24 22:13:43
-----Original Message-----
From: Dan Williams [mailto:dcbw@redhat.com]
Sent: Tuesday, July 24, 2012 2:37 PM
To: Stephen Hemminger
Cc: Olaf Hering; KY Srinivasan; gregkh@linuxfoundation.org; linux-
kernel@vger.kernel.org; devel@linuxdriverproject.org;
virtualization@lists.osdl.org; apw@canonical.com; netdev@vger.kernel.org;
ben@decadent.org.uk
Subject: Re: [PATCH 10/17] Tools: hv: Gather ipv[4,6] gateway information
On Tue, 2012-07-24 at 09:56 -0700, Stephen Hemminger wrote:
quoted
On Tue, 24 Jul 2012 18:53:59 +0200
Olaf Hering [off-list ref] wrote:
quoted
On Tue, Jul 24, Stephen Hemminger wrote:
quoted
On Tue, 24 Jul 2012 09:01:34 -0700
"K. Y. Srinivasan" [off-list ref] wrote:
This also has the benefit that ip is not called with absolute path, now
that distros move binaries around.
Olaf
It is also not hard to do the same thing with a little function
using libmnl
Yeah seriously, netlink anyone? You'll even get nicer error reporting
that way.
While I will be the first admit that using C API is always better (in C code),
in this particular instance I am not so sure. All I am doing is retrieving information
on default gateways. If there is an error, that is ok and this won't be reported
back to the host. Using the ip command significantly simplifies the code here.
Regards,
K. Y
From: Ben Hutchings <hidden> Date: 2012-07-24 23:38:21
On Tue, Jul 24, 2012 at 09:01:35AM -0700, K. Y. Srinivasan wrote:
Now gather DNS information. This information cannot be gathered in
a distro independent fashion. Invoke an external script (that can be
distro dependent) to gather the DNS information.
[...]
This is a weird way to build a string; why are you not using
snprintf()? Not to mention that interface names can contain several
characters that are special to the shell - in fact the only disallowed
characters are / and whitespace.
Also, the external script will not be useful to anything other than
hv_kvp_daemon, so it probably belongs somewhere under /usr/share.
Ben.
--
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert Camus
From: Ben Hutchings <hidden> Date: 2012-07-25 01:11:03
On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
quoted hunk
In preparation to implementing IP injection, cleanup the way we propagate
and handle errors both in the driver as well as in the user level daemon.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/hv_kvp.c | 112 +++++++++++++++++++++++++++++++++++++---------
include/linux/hyperv.h | 17 +++++---
tools/hv/hv_kvp_daemon.c | 70 +++++++++++++++-------------
3 files changed, 138 insertions(+), 61 deletions(-)
@@ -109,27 +154,52 @@ kvp_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp) { struct hv_kvp_msg *message; struct hv_kvp_msg_enumerate *data;+ int error = 0; message = (struct hv_kvp_msg *)msg->data;- switch (message->kvp_hdr.operation) {++ /*+ * If we are negotiating the version information+ * with the daemon; handle that first.+ */++ if (in_hand_shake) {+ if (kvp_handle_handshake(message))+ in_hand_shake = false;+ return;+ }++ /*+ * Based on the version of the daemon, we propagate errors from the+ * daemon differently.+ */++ data = &message->body.kvp_enum_data;++ switch (dm_reg_value) { case KVP_OP_REGISTER:- pr_info("KVP: user-mode registering done.\n");- kvp_register();- kvp_transaction.active = false;- hv_kvp_onchannelcallback(kvp_transaction.kvp_context);+ /*+ * Null string is used to pass back error condition.+ */+ if (!strlen(data->data.key))
Do we know that the key is null-terminated here? Shouldn't we just
check whether data->data.key[0] == 0?
+ error = HV_S_CONT;
break;
- default:
- data = &message->body.kvp_enum_data;
+ case KVP_OP_REGISTER1:
/*
- * Complete the transaction by forwarding the key value
- * to the host. But first, cancel the timeout.
+ * We use the message header information from
+ * the user level daemon to transmit errors.
*/
- if (cancel_delayed_work_sync(&kvp_work))
- kvp_respond_to_host(data->data.key,
- data->data.value,
- !strlen(data->data.key));
+ error = *((int *)(&message->kvp_hdr.operation));
[...]
What's with the casting (repeated in many other places)? Wouldn't it be
better to redefine struct hv_kvp_msg to start with something like:
union {
struct hv_kvp_hdr request;
int error;
} kvp_hdr;
Ben.
--
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.
From: Ben Hutchings <hidden> Date: 2012-07-25 01:14:18
On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
quoted hunk
Now gather sub-net information for the specified interface.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 31 +++++++++++++++++++++++++++++--
1 files changed, 29 insertions(+), 2 deletions(-)
@@ -534,6 +534,7 @@ kvp_get_ip_address(int family, char *if_name, int op,structifaddrs*ifap;structifaddrs*curp;intoffset=0;+intsn_offset=0;constchar*str;interror=0;char*buffer;
@@ -594,12 +595,38 @@ kvp_get_ip_address(int family, char *if_name, int op,*GatherinfootherthantheIPaddress.*IPaddressinfowillbegatheredlater.*/-if(curp->ifa_addr->sa_family==AF_INET)+if(curp->ifa_addr->sa_family==AF_INET){ip_buffer->addr_family|=ADDR_FAMILY_IPV4;-else+/*+*Getsubnetinfo.+*/+error=kvp_process_ip_address(+curp->ifa_netmask,+AF_INET,+(char*)+ip_buffer->sub_net,+length,+&sn_offset);
[...]
This is barely readable; why don't you indent the arguments by just one
extra tab?
Ben.
--
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.
From: Ben Hutchings <hidden> Date: 2012-07-25 01:25:07
On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
Implement the KVP verb - KVP_OP_SET_IP_INFO. This operation configures the
specified interface based on the given configuration. Since configuring
an interface is very distro specific, we invoke an external script to
configure the interface.
This style of string pasting is crazy; have you never heard of
fprintf()?
[...]
+ /*
+ * Set the configuration for the specified interface with
+ * the information provided. Since there is no standard
+ * way to configure an interface, we will have an external
+ * script that does the job of configuring the interface and
+ * flushing the configuration.
+ *
+ * The parameters passed to this external script are:
+ * 1. A configuration file that has the specified configuration.
+ *
+ * We will embed the name of the interface in the configuration
+ * file: ifcfg-ethx (where ethx is the interface name).
+ *
+ * Here is the format of the ip configuration file:
+ *
+ * HWADDR=macaddr
Is the interface supposed to be matched by name or by MAC address?
+ * BOOTPROTO=dhcp (dhcp enabled for the interface)
The BOOTPROTO line may or may not appear.
+ * NM_CONTROLLED=no (this interface will not be controlled by NM)
+ * PEERDNS=yes
I wonder what the point is of including constant lines in the file.
What is the external script supposed to do if it these apparent
constants change in future?
+ * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
+ * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
+ * IPV6NETMASK.
+ */
+
+ memset(if_file, 0, sizeof(if_file));
+ strcat(if_file, "/var/opt/hyperv/ifcfg-");
Like I said before about the key-value files, this should be under
/var/lib if the daemon is included in a distribution. You should
perhaps use a macro for the "/var/opt" part so it can be overridden
depending on whether it's built as a distribution or add-on package.
+ strcat(if_file, if_name);
+
+ file = fopen(if_file, "w");
+
+ if (file == NULL) {
+ syslog(LOG_ERR, "Failed to open config file");
+ return HV_E_FAIL;
+ }
+
+ /*
+ * First write out the MAC address.
+ */
+
+ mac_addr = kvp_if_name_to_mac(if_name);
+ if (mac_addr == NULL) {
+ error = HV_E_FAIL;
+ goto setval_error;
+ }
+
+ error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
+ if (error)
+ goto setval_error;
+
+ error = kvp_write_file(file, "ONBOOT", NULL, "yes");
+ if (error)
+ goto setval_error;
+
+ error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
+ if (error)
+ goto setval_error;
[...]
This line isn't mentioned in the above comment.
Ben.
--
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.
From: KY Srinivasan <kys@microsoft.com> Date: 2012-07-25 14:10:37
-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Tuesday, July 24, 2012 9:11 PM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
apw@canonical.com; netdev@vger.kernel.org
Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
quoted
In preparation to implementing IP injection, cleanup the way we propagate
and handle errors both in the driver as well as in the user level daemon.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/hv_kvp.c | 112 +++++++++++++++++++++++++++++++++++++-
{
struct hv_kvp_msg *message;
struct hv_kvp_msg_enumerate *data;
+ int error = 0;
message = (struct hv_kvp_msg *)msg->data;
- switch (message->kvp_hdr.operation) {
+
+ /*
+ * If we are negotiating the version information
+ * with the daemon; handle that first.
+ */
+
+ if (in_hand_shake) {
+ if (kvp_handle_handshake(message))
+ in_hand_shake = false;
+ return;
+ }
+
+ /*
+ * Based on the version of the daemon, we propagate errors from the
+ * daemon differently.
+ */
+
+ data = &message->body.kvp_enum_data;
+
+ switch (dm_reg_value) {
case KVP_OP_REGISTER:
- pr_info("KVP: user-mode registering done.\n");
- kvp_register();
- kvp_transaction.active = false;
- hv_kvp_onchannelcallback(kvp_transaction.kvp_context);
+ /*
+ * Null string is used to pass back error condition.
+ */
+ if (!strlen(data->data.key))
Do we know that the key is null-terminated here? Shouldn't we just
check whether data->data.key[0] == 0?
Yes, currently we do return null string to indicate error.
quoted
+ error = HV_S_CONT;
break;
- default:
- data = &message->body.kvp_enum_data;
+ case KVP_OP_REGISTER1:
/*
- * Complete the transaction by forwarding the key value
- * to the host. But first, cancel the timeout.
+ * We use the message header information from
+ * the user level daemon to transmit errors.
*/
- if (cancel_delayed_work_sync(&kvp_work))
- kvp_respond_to_host(data->data.key,
- data->data.value,
- !strlen(data->data.key));
+ error = *((int *)(&message->kvp_hdr.operation));
[...]
What's with the casting (repeated in many other places)? Wouldn't it be
better to redefine struct hv_kvp_msg to start with something like:
union {
struct hv_kvp_hdr request;
int error;
} kvp_hdr;
Agreed; will do.
Ben.
--
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.
From: KY Srinivasan <kys@microsoft.com> Date: 2012-07-25 14:11:09
-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Tuesday, July 24, 2012 9:14 PM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
apw@canonical.com; netdev@vger.kernel.org
Subject: Re: [PATCH 08/17] Tools: hv: Gather subnet information
On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
quoted
Now gather sub-net information for the specified interface.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 31 +++++++++++++++++++++++++++++--
1 files changed, 29 insertions(+), 2 deletions(-)
From: Ben Hutchings <hidden> Date: 2012-07-25 14:47:35
On Wed, Jul 25, 2012 at 02:10:05PM +0000, KY Srinivasan wrote:
quoted
-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Tuesday, July 24, 2012 9:11 PM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
apw@canonical.com; netdev@vger.kernel.org
Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
quoted
In preparation to implementing IP injection, cleanup the way we propagate
and handle errors both in the driver as well as in the user level daemon.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/hv_kvp.c | 112 +++++++++++++++++++++++++++++++++++++-
{
struct hv_kvp_msg *message;
struct hv_kvp_msg_enumerate *data;
+ int error = 0;
message = (struct hv_kvp_msg *)msg->data;
- switch (message->kvp_hdr.operation) {
+
+ /*
+ * If we are negotiating the version information
+ * with the daemon; handle that first.
+ */
+
+ if (in_hand_shake) {
+ if (kvp_handle_handshake(message))
+ in_hand_shake = false;
+ return;
+ }
+
+ /*
+ * Based on the version of the daemon, we propagate errors from the
+ * daemon differently.
+ */
+
+ data = &message->body.kvp_enum_data;
+
+ switch (dm_reg_value) {
case KVP_OP_REGISTER:
- pr_info("KVP: user-mode registering done.\n");
- kvp_register();
- kvp_transaction.active = false;
- hv_kvp_onchannelcallback(kvp_transaction.kvp_context);
+ /*
+ * Null string is used to pass back error condition.
+ */
+ if (!strlen(data->data.key))
Do we know that the key is null-terminated here? Shouldn't we just
check whether data->data.key[0] == 0?
Yes, currently we do return null string to indicate error.
[...]
So the kernel should assume userland input is always valid?
Ben.
--
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert Camus
From: KY Srinivasan <kys@microsoft.com> Date: 2012-07-25 14:48:58
Ben,
At the outset I want to thank you for taking the time to review this code. Given that Greg
has indicated that he will not be able to look at this patch set till 3.6 and the nature of review
comments I have gotten from you and others, I will re-spin this patch set to address all the
comments I have gotten to date. Specific responses to your comments are in-line.
Regards,
K. Y
-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Tuesday, July 24, 2012 9:25 PM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
apw@canonical.com; netdev@vger.kernel.org
Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
KVP_OP_SET_IP_INFO
On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
quoted
Implement the KVP verb - KVP_OP_SET_IP_INFO. This operation configures
the
quoted
specified interface based on the given configuration. Since configuring
an interface is very distro specific, we invoke an external script to
configure the interface.
This style of string pasting is crazy; have you never heard of
fprintf()?
[...]
quoted
+ /*
+ * Set the configuration for the specified interface with
+ * the information provided. Since there is no standard
+ * way to configure an interface, we will have an external
+ * script that does the job of configuring the interface and
+ * flushing the configuration.
+ *
+ * The parameters passed to this external script are:
+ * 1. A configuration file that has the specified configuration.
+ *
+ * We will embed the name of the interface in the configuration
+ * file: ifcfg-ethx (where ethx is the interface name).
+ *
+ * Here is the format of the ip configuration file:
+ *
+ * HWADDR=macaddr
Is the interface supposed to be matched by name or by MAC address?
I do not dictate that. My plan was to package all the information I have about
the interface and the desired configuration in a file and invoke the external
distro specific script to do its magic. This external script is free to ignore
what it does not need.
quoted
+ * BOOTPROTO=dhcp (dhcp enabled for the interface)
The BOOTPROTO line may or may not appear.
quoted
+ * NM_CONTROLLED=no (this interface will not be controlled by NM)
+ * PEERDNS=yes
I wonder what the point is of including constant lines in the file.
What is the external script supposed to do if it these apparent
constants change in future?
As you can see, I did my testing on a RHEL system and I was too lazy to create
a RHEL specific config file in the external script and so I ended up creating pretty much
the config file needed by RHEL in this daemon. All the external script had to do was to
simply copy this file into the right location and bring up the interface. So, if you prefer
that we not populate the config file with these constant lines, I can get rid of them.
As I noted earlier external scripts may not choose to use all the information in the file
that this daemon generates. What I have done here, simplifies the external script
at least for one distro.
+ * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
+ * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
+ * IPV6NETMASK.
+ */
+
+ memset(if_file, 0, sizeof(if_file));
+ strcat(if_file, "/var/opt/hyperv/ifcfg-");
Like I said before about the key-value files, this should be under
/var/lib if the daemon is included in a distribution. You should
perhaps use a macro for the "/var/opt" part so it can be overridden
depending on whether it's built as a distribution or add-on package.
I will make this a macro.
quoted
+ strcat(if_file, if_name);
+
+ file = fopen(if_file, "w");
+
+ if (file == NULL) {
+ syslog(LOG_ERR, "Failed to open config file");
+ return HV_E_FAIL;
+ }
+
+ /*
+ * First write out the MAC address.
+ */
+
+ mac_addr = kvp_if_name_to_mac(if_name);
+ if (mac_addr == NULL) {
+ error = HV_E_FAIL;
+ goto setval_error;
+ }
+
+ error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
+ if (error)
+ goto setval_error;
+
+ error = kvp_write_file(file, "ONBOOT", NULL, "yes");
+ if (error)
+ goto setval_error;
+
+ error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
+ if (error)
+ goto setval_error;
[...]
This line isn't mentioned in the above comment.
Ben.
--
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.
From: KY Srinivasan <kys@microsoft.com> Date: 2012-07-25 14:52:04
-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Wednesday, July 25, 2012 10:47 AM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
apw@canonical.com; netdev@vger.kernel.org
Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
On Wed, Jul 25, 2012 at 02:10:05PM +0000, KY Srinivasan wrote:
quoted
quoted
-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Tuesday, July 24, 2012 9:11 PM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
apw@canonical.com; netdev@vger.kernel.org
Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
quoted
In preparation to implementing IP injection, cleanup the way we propagate
and handle errors both in the driver as well as in the user level daemon.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/hv_kvp.c | 112
{
struct hv_kvp_msg *message;
struct hv_kvp_msg_enumerate *data;
+ int error = 0;
message = (struct hv_kvp_msg *)msg->data;
- switch (message->kvp_hdr.operation) {
+
+ /*
+ * If we are negotiating the version information
+ * with the daemon; handle that first.
+ */
+
+ if (in_hand_shake) {
+ if (kvp_handle_handshake(message))
+ in_hand_shake = false;
+ return;
+ }
+
+ /*
+ * Based on the version of the daemon, we propagate errors from the
+ * daemon differently.
+ */
+
+ data = &message->body.kvp_enum_data;
+
+ switch (dm_reg_value) {
case KVP_OP_REGISTER:
- pr_info("KVP: user-mode registering done.\n");
- kvp_register();
- kvp_transaction.active = false;
- hv_kvp_onchannelcallback(kvp_transaction.kvp_context);
+ /*
+ * Null string is used to pass back error condition.
+ */
+ if (!strlen(data->data.key))
Do we know that the key is null-terminated here? Shouldn't we just
check whether data->data.key[0] == 0?
Yes, currently we do return null string to indicate error.
[...]
So the kernel should assume userland input is always valid?
Good point! This is the existing code and this patch-set cleans up all
the error handling and would not have this problem.
K. Y
From: KY Srinivasan <kys@microsoft.com> Date: 2012-07-29 23:07:30
-----Original Message-----
From: Greg KH [mailto:gregkh@linuxfoundation.org]
Sent: Tuesday, July 24, 2012 11:54 AM
To: KY Srinivasan
Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
virtualization@lists.osdl.org; olaf@aepfle.de; apw@canonical.com;
netdev@vger.kernel.org; ben@decadent.org.uk
Subject: Re: [PATCH 00/17] drivers: hv: kvp
On Tue, Jul 24, 2012 at 09:01:12AM -0700, K. Y. Srinivasan wrote:
quoted
This patchset expands the KVP (Key Value Pair) functionality to
implement the mechanism to GET/SET IP addresses in the guest. This
functionality is used in Windows Server 2012 to implement VM
replication functionality. The way IP configuration information
is managed is distro specific. Based on the feedback I have gotten
from Olaf, Greg, Steve, Ben and Mairus, I have chosen to seperate
distro specific code from this patch-set. Most of the GET operation
can be implemented in a way that is completely distro independent and
I have implemented that as such and is included in this patch-set.
Some of the attributes that can only be fetched in a distro
dependent way as well the mechanism for configuring an interface
(the SET operation) that is clearly distro specific is to be
implemented via external scripts that will be invoked via the KVP
code. We define here the interface to these scripts.
Adding support for IP injection resulted in some changes to the
protocol between the user level daemon and the kernel driver.
These changes have been implemented in way that would retain
compatibility with older daemons. I would like to thank Olaf and
Greg for pointing out the compatibility issue.
Due to this being the middle of the merge window, I will not be able to
look at this until after 3.6-rc1 is out.
Thanks Greg. In the meantime, I have addressed all the comments that both Olaf
and Ben have posted on this patch-set. Since addressing these comments changed
some data structures, I think it will be best if you dropped this patch-set. I will post the
updated patch-set shortly.
Regards,
K. Y
+
+ file = fopen(addr_file, "r");
+ if (file == NULL)
+ return NULL;
+
+ p = fgets(buf, sizeof(buf), file);
+ if (p) {
+ x = strchr(p, '\n');
+ if (x)
+ *x = '\0';
+ for (i = 0; i < strlen(p); i++)
+ p[i] = toupper(p[i]);
+ mac_addr = strdup(p);
+ }
+
+ fclose(file);
+ return mac_addr;
+}
+
+
static void kvp_process_ipconfig_file(char *cmd,
char *config_buf, int len,
int element_size, int offset)
@@ -800,6 +910,314 @@ getaddr_done: }+static int expand_ipv6(char *addr, int type)+{+ int ret;+ struct in6_addr v6_addr;++ ret = inet_pton(AF_INET6, addr, &v6_addr);++ if (ret != 1) {+ if (type == NETMASK)+ return 1;+ return 0;+ }++ sprintf(addr, "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:"+ "%02x%02x:%02x%02x:%02x%02x",+ (int)v6_addr.s6_addr[0], (int)v6_addr.s6_addr[1],+ (int)v6_addr.s6_addr[2], (int)v6_addr.s6_addr[3],+ (int)v6_addr.s6_addr[4], (int)v6_addr.s6_addr[5],+ (int)v6_addr.s6_addr[6], (int)v6_addr.s6_addr[7],+ (int)v6_addr.s6_addr[8], (int)v6_addr.s6_addr[9],+ (int)v6_addr.s6_addr[10], (int)v6_addr.s6_addr[11],+ (int)v6_addr.s6_addr[12], (int)v6_addr.s6_addr[13],+ (int)v6_addr.s6_addr[14], (int)v6_addr.s6_addr[15]);++ return 1;++}++static int is_ipv4(char *addr)+{+ int ret;+ struct in_addr ipv4_addr;++ ret = inet_pton(AF_INET, addr, &ipv4_addr);++ if (ret == 1)+ return 1;+ return 0;+}++static int parse_ip_val_buffer(char *in_buf, int *offset,+ char *out_buf, int out_len)+{+ char *x;+ char *start;++ /*+ * in_buf has sequence of characters that are seperated by+ * the character ';'. The last sequence does not have the+ * terminating ";" character.+ */+ start = in_buf + *offset;++ x = strchr(start, ';');+ if (x)+ *x = 0;+ else+ x = start + strlen(start);++ if (strlen(start) != 0) {+ int i = 0;+ /*+ * Get rid of leading spaces.+ */+ while (start[i] == ' ')+ i++;++ if ((x - start) <= out_len) {+ strcpy(out_buf, (start + i));+ *offset += (x - start) + 1;+ return 1;+ }+ }+ return 0;+}++static int kvp_write_file(FILE *f, char *s1, char *s2, char *s3)+{+ char str[256];+ int error;++ memset(str, 0, sizeof(str));+ strcat(str, s1);+ if (s2 != NULL)+ strcat(str, s2);+ strcat(str, "=");+ strcat(str, s3);+ strcat(str, "\n");++ error = fputs(str, f);+ if (error == EOF)+ return HV_E_FAIL;++ return 0;+}+++static int process_ip_string(FILE *f, char *ip_string, int type)+{+ int error = 0;+ char addr[INET6_ADDRSTRLEN];+ int i = 0;+ int j = 0;+ char str[256];+ char sub_str[10];+ int offset = 0;++ memset(addr, 0, sizeof(addr));++ while (parse_ip_val_buffer(ip_string, &offset, addr,+ (MAX_IP_ADDR_SIZE * 2))) {+ memset(sub_str, 0, sizeof(sub_str));+ memset(str, 0, sizeof(str));++ if (is_ipv4(addr)) {+ switch (type) {+ case IPADDR:+ strcat(str, "IPADDR");+ break;+ case NETMASK:+ strcat(str, "NETMASK");+ break;+ case GATEWAY:+ strcat(str, "GATEWAY");+ break;+ case DNS:+ strcat(str, "DNS");+ break;+ }+ if (i != 0) {+ if (type != DNS)+ sprintf(sub_str, "_%d", i++);+ else+ sprintf(sub_str, "%d", ++i);+ } else if (type == DNS) {+ sprintf(sub_str, "%d", ++i);+ }+++ } else if (expand_ipv6(addr, type)) {+ switch (type) {+ case IPADDR:+ strcat(str, "IPV6ADDR");+ break;+ case NETMASK:+ strcat(str, "IPV6NETMASK");+ break;+ case GATEWAY:+ strcat(str, "IPV6_DEFAULTGW");+ break;+ case DNS:+ strcat(str, "DNS");+ break;+ }+ if ((j != 0) || (type == DNS)) {+ if (type != DNS)+ sprintf(sub_str, "_%d", j++);+ else+ sprintf(sub_str, "%d", ++i);+ } else if (type == DNS) {+ sprintf(sub_str, "%d", ++i);+ }+ } else {+ return HV_INVALIDARG;+ }++ error = kvp_write_file(f, str, sub_str, addr);+ if (error)+ return error;+ memset(addr, 0, sizeof(addr));+ }++ return 0;+}++static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)+{+ int error = 0;+ char if_file[50];+ FILE *file;+ char cmd[512];+ char *mac_addr;++ /*+ * Set the configuration for the specified interface with+ * the information provided. Since there is no standard+ * way to configure an interface, we will have an external+ * script that does the job of configuring the interface and+ * flushing the configuration.+ *+ * The parameters passed to this external script are:+ * 1. A configuration file that has the specified configuration.+ *+ * We will embed the name of the interface in the configuration+ * file: ifcfg-ethx (where ethx is the interface name).+ *+ * Here is the format of the ip configuration file:+ *+ * HWADDR=macaddr+ * BOOTPROTO=dhcp (dhcp enabled for the interface)+ * NM_CONTROLLED=no (this interface will not be controlled by NM)+ * PEERDNS=yes+ * IPADDR_x=ipaddr+ * NETMASK_x=netmask+ * GATEWAY_x=gateway+ * DNSx=dns+ *+ * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be+ * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as+ * IPV6NETMASK.+ */++ memset(if_file, 0, sizeof(if_file));+ strcat(if_file, "/var/opt/hyperv/ifcfg-");+ strcat(if_file, if_name);++ file = fopen(if_file, "w");++ if (file == NULL) {+ syslog(LOG_ERR, "Failed to open config file");+ return HV_E_FAIL;+ }++ /*+ * First write out the MAC address.+ */++ mac_addr = kvp_if_name_to_mac(if_name);+ if (mac_addr == NULL) {+ error = HV_E_FAIL;+ goto setval_error;+ }++ error = kvp_write_file(file, "HWADDR", NULL, mac_addr);+ if (error)+ goto setval_error;++ error = kvp_write_file(file, "ONBOOT", NULL, "yes");+ if (error)+ goto setval_error;++ error = kvp_write_file(file, "IPV6INIT", NULL, "yes");+ if (error)+ goto setval_error;++ error = kvp_write_file(file, "NM_CONTROLLED", NULL, "no");+ if (error)+ goto setval_error;++ error = kvp_write_file(file, "PEERDNS", NULL, "yes");+ if (error)+ goto setval_error;++ if (new_val->dhcp_enabled) {+ error = kvp_write_file(file, "BOOTPROTO", NULL, "dhcp");+ if (error)+ goto setval_error;++ /*+ * We are done!.+ */+ goto setval_done;+ }++ /*+ * Write the configuration for ipaddress, netmask, gateway and+ * name servers.+ */++ error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);+ if (error)+ goto setval_error;++ error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);+ if (error)+ goto setval_error;++ error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);+ if (error)+ goto setval_error;++ error = process_ip_string(file, (char *)new_val->dns_addr, DNS);+ if (error)+ goto setval_error;++setval_done:+ free(mac_addr);+ fclose(file);++ /*+ * Now that we have populated the configuration file,+ * invoke the external script to do its magic.+ */++ memset(cmd, 0, sizeof(cmd));+ strcat(cmd, "/sbin/hv_set_ifconfig ");+ strcat(cmd, if_file);
The new patch should use "%s %s", not "%s%s" as format string.
From: Olaf Hering <hidden> Date: 2012-07-30 18:03:31
On Tue, Jul 24, K. Y. Srinivasan wrote:
+ /*
+ * Set the configuration for the specified interface with
+ * the information provided. Since there is no standard
+ * way to configure an interface, we will have an external
+ * script that does the job of configuring the interface and
+ * flushing the configuration.
+ *
+ * The parameters passed to this external script are:
+ * 1. A configuration file that has the specified configuration.
Maybe this should be written as 'A info file that has the requested
network configuration' or something like that.
+ *
+ * We will embed the name of the interface in the configuration
+ * file: ifcfg-ethx (where ethx is the interface name).
I think the intention here is to use the generated file as is. Depending
on the distro in the guest the file may need some processing. So I think
the actual interface name should also be part of the file.
+ *
+ * Here is the format of the ip configuration file:
+ *
+ * HWADDR=macaddr
+ * BOOTPROTO=dhcp (dhcp enabled for the interface)
While BOOTPROTO= is used in current network config files, its meaning
there is unrelated to what its meant here. Here it means DHCP=yes/no, so
I think the file should contain just that. And as the code is written
now BOOTPROTO= is optional, which is not mentioned in the comment.
+ * NM_CONTROLLED=no (this interface will not be controlled by NM)
I think this is not up to kvp_deamon to decide what controls the
interface. Maybe one day NM is sufficiently advanced technology that it
can cope with such requests?
The helper script should decide if the NM flag should be written to the
final config file.
From: KY Srinivasan <kys@microsoft.com> Date: 2012-07-30 18:33:39
-----Original Message-----
From: Olaf Hering [mailto:olaf@aepfle.de]
Sent: Monday, July 30, 2012 2:03 PM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
ben@decadent.org.uk
Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
KVP_OP_SET_IP_INFO
On Tue, Jul 24, K. Y. Srinivasan wrote:
quoted
+ /*
+ * Set the configuration for the specified interface with
+ * the information provided. Since there is no standard
+ * way to configure an interface, we will have an external
+ * script that does the job of configuring the interface and
+ * flushing the configuration.
+ *
+ * The parameters passed to this external script are:
+ * 1. A configuration file that has the specified configuration.
Maybe this should be written as 'A info file that has the requested
network configuration' or something like that.
That is the idea. This configuration file simply reflects all the information we have
perhaps with some additional constant information. The script is free to ignore what
it does not need.
quoted
+ *
+ * We will embed the name of the interface in the configuration
+ * file: ifcfg-ethx (where ethx is the interface name).
I think the intention here is to use the generated file as is. Depending
on the distro in the guest the file may need some processing. So I think
the actual interface name should also be part of the file.
That is not the intention although on some distros, the format of this file
may be closer to the distro specific configuration file than others. I will
however include the name of the interface in the file as well.
quoted
+ *
+ * Here is the format of the ip configuration file:
+ *
+ * HWADDR=macaddr
+ * BOOTPROTO=dhcp (dhcp enabled for the interface)
While BOOTPROTO= is used in current network config files, its meaning
there is unrelated to what its meant here. Here it means DHCP=yes/no, so
I think the file should contain just that. And as the code is written
now BOOTPROTO= is optional, which is not mentioned in the comment.
I will fix this.
quoted
+ * NM_CONTROLLED=no (this interface will not be controlled by NM)
I think this is not up to kvp_deamon to decide what controls the
interface. Maybe one day NM is sufficiently advanced technology that it
can cope with such requests?
The helper script should decide if the NM flag should be written to the
final config file.
As I noted earlier, the external script can choose to interpret the contents of this
file in a way that it makes sense for the distribution. Maybe, I will get rid of all the
constant information.
+
+ file = fopen(addr_file, "r");
+ if (file == NULL)
+ return NULL;
+
+ p = fgets(buf, sizeof(buf), file);
+ if (p) {
+ x = strchr(p, '\n');
+ if (x)
+ *x = '\0';
+ for (i = 0; i < strlen(p); i++)
+ p[i] = toupper(p[i]);
+ mac_addr = strdup(p);
+ }
+
+ fclose(file);
+ return mac_addr;
+}
+
+
static void kvp_process_ipconfig_file(char *cmd,
char *config_buf, int len,
int element_size, int offset)
@@ -800,6 +910,314 @@ getaddr_done: }+static int expand_ipv6(char *addr, int type)+{+ int ret;+ struct in6_addr v6_addr;++ ret = inet_pton(AF_INET6, addr, &v6_addr);++ if (ret != 1) {+ if (type == NETMASK)+ return 1;+ return 0;+ }++ sprintf(addr, "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:"+ "%02x%02x:%02x%02x:%02x%02x",+ (int)v6_addr.s6_addr[0], (int)v6_addr.s6_addr[1],+ (int)v6_addr.s6_addr[2], (int)v6_addr.s6_addr[3],+ (int)v6_addr.s6_addr[4], (int)v6_addr.s6_addr[5],+ (int)v6_addr.s6_addr[6], (int)v6_addr.s6_addr[7],+ (int)v6_addr.s6_addr[8], (int)v6_addr.s6_addr[9],+ (int)v6_addr.s6_addr[10], (int)v6_addr.s6_addr[11],+ (int)v6_addr.s6_addr[12], (int)v6_addr.s6_addr[13],+ (int)v6_addr.s6_addr[14], (int)v6_addr.s6_addr[15]);++ return 1;++}++static int is_ipv4(char *addr)+{+ int ret;+ struct in_addr ipv4_addr;++ ret = inet_pton(AF_INET, addr, &ipv4_addr);++ if (ret == 1)+ return 1;+ return 0;+}++static int parse_ip_val_buffer(char *in_buf, int *offset,+ char *out_buf, int out_len)+{+ char *x;+ char *start;++ /*+ * in_buf has sequence of characters that are seperated by+ * the character ';'. The last sequence does not have the+ * terminating ";" character.+ */+ start = in_buf + *offset;++ x = strchr(start, ';');+ if (x)+ *x = 0;+ else+ x = start + strlen(start);++ if (strlen(start) != 0) {+ int i = 0;+ /*+ * Get rid of leading spaces.+ */+ while (start[i] == ' ')+ i++;++ if ((x - start) <= out_len) {+ strcpy(out_buf, (start + i));+ *offset += (x - start) + 1;+ return 1;+ }+ }+ return 0;+}++static int kvp_write_file(FILE *f, char *s1, char *s2, char *s3)+{+ char str[256];+ int error;++ memset(str, 0, sizeof(str));+ strcat(str, s1);+ if (s2 != NULL)+ strcat(str, s2);+ strcat(str, "=");+ strcat(str, s3);+ strcat(str, "\n");++ error = fputs(str, f);+ if (error == EOF)+ return HV_E_FAIL;++ return 0;+}+++static int process_ip_string(FILE *f, char *ip_string, int type)+{+ int error = 0;+ char addr[INET6_ADDRSTRLEN];+ int i = 0;+ int j = 0;+ char str[256];+ char sub_str[10];+ int offset = 0;++ memset(addr, 0, sizeof(addr));++ while (parse_ip_val_buffer(ip_string, &offset, addr,+ (MAX_IP_ADDR_SIZE * 2))) {+ memset(sub_str, 0, sizeof(sub_str));+ memset(str, 0, sizeof(str));++ if (is_ipv4(addr)) {+ switch (type) {+ case IPADDR:+ strcat(str, "IPADDR");+ break;+ case NETMASK:+ strcat(str, "NETMASK");+ break;+ case GATEWAY:+ strcat(str, "GATEWAY");+ break;+ case DNS:+ strcat(str, "DNS");+ break;+ }+ if (i != 0) {+ if (type != DNS)+ sprintf(sub_str, "_%d", i++);+ else+ sprintf(sub_str, "%d", ++i);+ } else if (type == DNS) {+ sprintf(sub_str, "%d", ++i);+ }+++ } else if (expand_ipv6(addr, type)) {+ switch (type) {+ case IPADDR:+ strcat(str, "IPV6ADDR");+ break;+ case NETMASK:+ strcat(str, "IPV6NETMASK");+ break;+ case GATEWAY:+ strcat(str, "IPV6_DEFAULTGW");+ break;+ case DNS:+ strcat(str, "DNS");+ break;+ }+ if ((j != 0) || (type == DNS)) {+ if (type != DNS)+ sprintf(sub_str, "_%d", j++);+ else+ sprintf(sub_str, "%d", ++i);+ } else if (type == DNS) {+ sprintf(sub_str, "%d", ++i);+ }+ } else {+ return HV_INVALIDARG;+ }++ error = kvp_write_file(f, str, sub_str, addr);+ if (error)+ return error;+ memset(addr, 0, sizeof(addr));+ }++ return 0;+}++static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value
*new_val)
quoted
+{
+ int error = 0;
+ char if_file[50];
+ FILE *file;
+ char cmd[512];
+ char *mac_addr;
+
+ /*
+ * Set the configuration for the specified interface with
+ * the information provided. Since there is no standard
+ * way to configure an interface, we will have an external
+ * script that does the job of configuring the interface and
+ * flushing the configuration.
+ *
+ * The parameters passed to this external script are:
+ * 1. A configuration file that has the specified configuration.
+ *
+ * We will embed the name of the interface in the configuration
+ * file: ifcfg-ethx (where ethx is the interface name).
+ *
+ * Here is the format of the ip configuration file:
+ *
+ * HWADDR=macaddr
+ * BOOTPROTO=dhcp (dhcp enabled for the interface)
+ * NM_CONTROLLED=no (this interface will not be controlled by NM)
+ * PEERDNS=yes
+ * IPADDR_x=ipaddr
+ * NETMASK_x=netmask
+ * GATEWAY_x=gateway
+ * DNSx=dns
+ *
+ * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
+ * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
+ * IPV6NETMASK.
+ */
+
+ memset(if_file, 0, sizeof(if_file));
+ strcat(if_file, "/var/opt/hyperv/ifcfg-");
+ strcat(if_file, if_name);
+
+ file = fopen(if_file, "w");
+
+ if (file == NULL) {
+ syslog(LOG_ERR, "Failed to open config file");
+ return HV_E_FAIL;
+ }
+
+ /*
+ * First write out the MAC address.
+ */
+
+ mac_addr = kvp_if_name_to_mac(if_name);
+ if (mac_addr == NULL) {
+ error = HV_E_FAIL;
+ goto setval_error;
+ }
+
+ error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
+ if (error)
+ goto setval_error;
+
+ error = kvp_write_file(file, "ONBOOT", NULL, "yes");
+ if (error)
+ goto setval_error;
+
+ error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
+ if (error)
+ goto setval_error;
+
+ error = kvp_write_file(file, "NM_CONTROLLED", NULL, "no");
+ if (error)
+ goto setval_error;
+
+ error = kvp_write_file(file, "PEERDNS", NULL, "yes");
+ if (error)
+ goto setval_error;
+
+ if (new_val->dhcp_enabled) {
+ error = kvp_write_file(file, "BOOTPROTO", NULL, "dhcp");
+ if (error)
+ goto setval_error;
+
+ /*
+ * We are done!.
+ */
+ goto setval_done;
+ }
+
+ /*
+ * Write the configuration for ipaddress, netmask, gateway and
+ * name servers.
+ */
+
+ error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
+ if (error)
+ goto setval_error;
+
+ error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
+ if (error)
+ goto setval_error;
+
+ error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
+ if (error)
+ goto setval_error;
+
+ error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
+ if (error)
+ goto setval_error;
+
+setval_done:
+ free(mac_addr);
+ fclose(file);
+
+ /*
+ * Now that we have populated the configuration file,
+ * invoke the external script to do its magic.
+ */
+
+ memset(cmd, 0, sizeof(cmd));
+ strcat(cmd, "/sbin/hv_set_ifconfig ");
+ strcat(cmd, if_file);
The new patch should use "%s %s", not "%s%s" as format string.
From: Ben Hutchings <hidden> Date: 2012-07-30 19:19:25
On Mon, Jul 30, 2012 at 06:32:15PM +0000, KY Srinivasan wrote:
quoted
-----Original Message-----
From: Olaf Hering [mailto:olaf@aepfle.de]
Sent: Monday, July 30, 2012 2:03 PM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
ben@decadent.org.uk
Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
KVP_OP_SET_IP_INFO
On Tue, Jul 24, K. Y. Srinivasan wrote:
quoted
+ /*
+ * Set the configuration for the specified interface with
+ * the information provided. Since there is no standard
+ * way to configure an interface, we will have an external
+ * script that does the job of configuring the interface and
+ * flushing the configuration.
+ *
+ * The parameters passed to this external script are:
+ * 1. A configuration file that has the specified configuration.
Maybe this should be written as 'A info file that has the requested
network configuration' or something like that.
That is the idea. This configuration file simply reflects all the
information we have perhaps with some additional constant
information. The script is free to ignore what it does not need.
[...]
This does not strike me as a sensible interface. If scripts are
'free to ignore' information then the KVP interface becomes unreliable
as a means for managing networking on Linux guests. I would suggest
that at the least the script should be able to report that it did not
recognise some parts of the configuration. This would be logged
and/or reported back to the hypervisor.
(This is separate from the issue of constant configuration lines;
for some distributions the script might recognise but ignore them
because they have no use on that distribution. I don't see the
point in constant lines, but they don't seem to result in any
unreliability.)
Ben.
--
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert Camus
From: KY Srinivasan <kys@microsoft.com> Date: 2012-07-31 10:34:56
-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Monday, July 30, 2012 3:19 PM
To: KY Srinivasan
Cc: Olaf Hering; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org
Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
KVP_OP_SET_IP_INFO
On Mon, Jul 30, 2012 at 06:32:15PM +0000, KY Srinivasan wrote:
quoted
quoted
-----Original Message-----
From: Olaf Hering [mailto:olaf@aepfle.de]
Sent: Monday, July 30, 2012 2:03 PM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
ben@decadent.org.uk
Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
KVP_OP_SET_IP_INFO
On Tue, Jul 24, K. Y. Srinivasan wrote:
quoted
+ /*
+ * Set the configuration for the specified interface with
+ * the information provided. Since there is no standard
+ * way to configure an interface, we will have an external
+ * script that does the job of configuring the interface and
+ * flushing the configuration.
+ *
+ * The parameters passed to this external script are:
+ * 1. A configuration file that has the specified configuration.
Maybe this should be written as 'A info file that has the requested
network configuration' or something like that.
That is the idea. This configuration file simply reflects all the
information we have perhaps with some additional constant
information. The script is free to ignore what it does not need.
[...]
This does not strike me as a sensible interface. If scripts are
'free to ignore' information then the KVP interface becomes unreliable
as a means for managing networking on Linux guests. I would suggest
that at the least the script should be able to report that it did not
recognise some parts of the configuration. This would be logged
and/or reported back to the hypervisor.
(This is separate from the issue of constant configuration lines;
for some distributions the script might recognise but ignore them
because they have no use on that distribution. I don't see the
point in constant lines, but they don't seem to result in any
unreliability.)
Ben,
I see your point. I have cleaned up the contents of the KVP produced
configuration file to not include constant information that can be
auto generated by the distro specific script if it needs to. Also, I have
tried to make the documentation of the contents of the file a little
better. I will send out these new patches soon. Still, there is a possibility that
some of the content of this file may be redundant on a specific distro and I think
that should be fine. For instance, per Olaf's suggestion, I have included a line that
specifies the interface name in the file (in addition to the mac address). Given the
current format of the name of the config file (where the interface name is embedded
in the config file name, this additional name entry may be redundant on some distros.
Once again, thank you for taking the time to review this code.
Regards,
K. Y