> -----Original Message----- > From: Ben Hutchings [mailto:ben@xxxxxxxxxxxxxxx] > Sent: Tuesday, July 24, 2012 9:11 PM > To: KY Srinivasan > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; olaf@xxxxxxxxx; > apw@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx > 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: > > 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@xxxxxxxxxxxxx> > > Reviewed-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> > > --- > > 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(-) > > > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c > > index 0012eed..9b7fc4a 100644 > > --- a/drivers/hv/hv_kvp.c > > +++ b/drivers/hv/hv_kvp.c > [...] > > @@ -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? Yes, currently we do return null string to indicate error. > > > + 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. _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
