Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1393201 > unrolled thread

[PATCH net-next] block/drbd: use nla_put_u64_64bit()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next] block/drbd: use nla_put_u64_64bit() Nicolas Dichtel <nicolas.dichtel@6wind.com> - 2016-05-03 11:00 +0200
    Re: [PATCH net-next] block/drbd: use nla_put_u64_64bit() Nicolas Dichtel <nicolas.dichtel@6wind.com> - 2016-05-03 11:30 +0200
      [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() Nicolas Dichtel <nicolas.dichtel@6wind.com> - 2016-05-03 11:40 +0200
        Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() Lars Ellenberg <lars.ellenberg@linbit.com> - 2016-05-03 12:10 +0200
          Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() Nicolas Dichtel <nicolas.dichtel@6wind.com> - 2016-05-03 14:10 +0200
          Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() David Miller <davem@davemloft.net> - 2016-05-03 18:10 +0200
            Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() Lars Ellenberg <lars.ellenberg@linbit.com> - 2016-05-04 11:10 +0200
              Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() Nicolas Dichtel <nicolas.dichtel@6wind.com> - 2016-05-04 14:50 +0200
                Re: [Drbd-dev] [PATCH net-next v2] block/drbd: use  nla_put_u64_64bit() Lars Ellenberg <lars.ellenberg@linbit.com> - 2016-05-04 15:00 +0200
                Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-04 16:30 +0200
                  Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() David Miller <davem@davemloft.net> - 2016-05-04 19:00 +0200
                    Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() Eric Dumazet <eric.dumazet@gmail.com> - 2016-05-04 19:20 +0200
                Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() David Miller <davem@davemloft.net> - 2016-05-04 18:50 +0200
          Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit() David Miller <davem@davemloft.net> - 2016-05-03 18:10 +0200
          [PATCH net-next v3] block/drbd: align properly u64 in nl messages Nicolas Dichtel <nicolas.dichtel@6wind.com> - 2016-05-09 11:50 +0200
            Re: [PATCH net-next v3] block/drbd: align properly u64 in nl messages Lars Ellenberg <lars.ellenberg@linbit.com> - 2016-05-09 15:20 +0200
              Re: [PATCH net-next v3] block/drbd: align properly u64 in nl messages Nicolas Dichtel <nicolas.dichtel@6wind.com> - 2016-05-10 11:20 +0200
                Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in  nl messages Lars Ellenberg <lars.ellenberg@linbit.com> - 2016-05-10 11:50 +0200
                  Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in  nl messages Nicolas Dichtel <nicolas.dichtel@6wind.com> - 2016-05-10 12:10 +0200
                  Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64  in nl messages David Miller <davem@davemloft.net> - 2016-05-10 17:50 +0200
                    Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in  nl messages Lars Ellenberg <lars.ellenberg@linbit.com> - 2016-05-10 21:10 +0200
                      Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64  in nl messages David Miller <davem@davemloft.net> - 2016-05-10 21:30 +0200

Page 1 of 2  [1] 2  Next page →

#1393201 — [PATCH net-next] block/drbd: use nla_put_u64_64bit()

I had to define an intermediate function (nla_magic_put_flag()) because
handlers in genl_magic_struct.h expect a function with three arguments.

Note that this patch is only compile-tested.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 drivers/block/drbd/drbd_nl.c      | 29 +++++++++++++++++------------
 include/linux/drbd_genl.h         |  1 +
 include/linux/genl_magic_struct.h |  4 ++++
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index 1fd1dccebb6b..22ec2ede4110 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -3633,14 +3633,15 @@ static int nla_put_status_info(struct sk_buff *skb, struct drbd_device *device,
 		goto nla_put_failure;
 	if (nla_put_u32(skb, T_sib_reason, sib ? sib->sib_reason : SIB_GET_STATUS_REPLY) ||
 	    nla_put_u32(skb, T_current_state, device->state.i) ||
-	    nla_put_u64(skb, T_ed_uuid, device->ed_uuid) ||
-	    nla_put_u64(skb, T_capacity, drbd_get_capacity(device->this_bdev)) ||
-	    nla_put_u64(skb, T_send_cnt, device->send_cnt) ||
-	    nla_put_u64(skb, T_recv_cnt, device->recv_cnt) ||
-	    nla_put_u64(skb, T_read_cnt, device->read_cnt) ||
-	    nla_put_u64(skb, T_writ_cnt, device->writ_cnt) ||
-	    nla_put_u64(skb, T_al_writ_cnt, device->al_writ_cnt) ||
-	    nla_put_u64(skb, T_bm_writ_cnt, device->bm_writ_cnt) ||
+	    nla_put_u64_64bit(skb, T_ed_uuid, device->ed_uuid, T_pad) ||
+	    nla_put_u64_64bit(skb, T_capacity,
+			      drbd_get_capacity(device->this_bdev), T_pad) ||
+	    nla_put_u64_64bit(skb, T_send_cnt, device->send_cnt, T_pad) ||
+	    nla_put_u64_64bit(skb, T_recv_cnt, device->recv_cnt, T_pad) ||
+	    nla_put_u64_64bit(skb, T_read_cnt, device->read_cnt, T_pad) ||
+	    nla_put_u64_64bit(skb, T_writ_cnt, device->writ_cnt, T_pad) ||
+	    nla_put_u64_64bit(skb, T_al_writ_cnt, device->al_writ_cnt, T_pad) ||
+	    nla_put_u64_64bit(skb, T_bm_writ_cnt, device->bm_writ_cnt, T_pad) ||
 	    nla_put_u32(skb, T_ap_bio_cnt, atomic_read(&device->ap_bio_cnt)) ||
 	    nla_put_u32(skb, T_ap_pending_cnt, atomic_read(&device->ap_pending_cnt)) ||
 	    nla_put_u32(skb, T_rs_pending_cnt, atomic_read(&device->rs_pending_cnt)))
@@ -3657,13 +3658,17 @@ static int nla_put_status_info(struct sk_buff *skb, struct drbd_device *device,
 			goto nla_put_failure;
 
 		if (nla_put_u32(skb, T_disk_flags, device->ldev->md.flags) ||
-		    nla_put_u64(skb, T_bits_total, drbd_bm_bits(device)) ||
-		    nla_put_u64(skb, T_bits_oos, drbd_bm_total_weight(device)))
+		    nla_put_u64_64bit(skb, T_bits_total, drbd_bm_bits(device),
+				      T_pad) ||
+		    nla_put_u64_64bit(skb, T_bits_oos,
+				      drbd_bm_total_weight(device), T_pad))
 			goto nla_put_failure;
 		if (C_SYNC_SOURCE <= device->state.conn &&
 		    C_PAUSED_SYNC_T >= device->state.conn) {
-			if (nla_put_u64(skb, T_bits_rs_total, device->rs_total) ||
-			    nla_put_u64(skb, T_bits_rs_failed, device->rs_failed))
+			if (nla_put_u64_64bit(skb, T_bits_rs_total,
+					      device->rs_total, T_pad) ||
+			    nla_put_u64_64bit(skb, T_bits_rs_failed,
+					      device->rs_failed, T_pad))
 				goto nla_put_failure;
 		}
 	}
diff --git a/include/linux/drbd_genl.h b/include/linux/drbd_genl.h
index 2d0e5ad5de9d..8d327d8fbbc2 100644
--- a/include/linux/drbd_genl.h
+++ b/include/linux/drbd_genl.h
@@ -227,6 +227,7 @@ GENL_struct(DRBD_NLA_STATE_INFO, 8, state_info,
 	__u32_field(21,                      0, ap_bio_cnt)
 	__u32_field(22,                      0, ap_pending_cnt)
 	__u32_field(23,                      0, rs_pending_cnt)
+	__unspec_field(24,                   0, pad)
 )
 
 GENL_struct(DRBD_NLA_START_OV_PARMS, 9, start_ov_parms,
diff --git a/include/linux/genl_magic_struct.h b/include/linux/genl_magic_struct.h
index eecd19b37001..fde46be8fc40 100644
--- a/include/linux/genl_magic_struct.h
+++ b/include/linux/genl_magic_struct.h
@@ -61,11 +61,15 @@ extern void CONCAT_(GENL_MAGIC_FAMILY, _genl_unregister)(void);
  */
 
 /* MAGIC helpers							{{{2 */
+#define nla_magic_put_flag(skb, attr, val) nla_put_flag(skb, attr)
 
 /* possible field types */
 #define __flg_field(attr_nr, attr_flag, name) \
 	__field(attr_nr, attr_flag, name, NLA_U8, char, \
 			nla_get_u8, nla_put_u8, false)
+#define __unspec_field(attr_nr, attr_flag, name)	\
+	__field(attr_nr, attr_flag, name, NLA_UNSPEC, unsigned char, \
+			nla_get_flag, nla_magic_put_flag, false)
 #define __u8_field(attr_nr, attr_flag, name)	\
 	__field(attr_nr, attr_flag, name, NLA_U8, unsigned char, \
 			nla_get_u8, nla_put_u8, false)
-- 
2.8.1

[toc] | [next] | [standalone]

#1393227

Le 03/05/2016 10:50, Nicolas Dichtel a écrit :
> I had to define an intermediate function (nla_magic_put_flag()) because
> handlers in genl_magic_struct.h expect a function with three arguments.
> 
> Note that this patch is only compile-tested.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Please, drop it. I will send another version to handle all cases.

[toc] | [prev] | [next] | [standalone]

#1393243 — [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

Two new handlers have been defined in genl_magic_ headers:
 - __field2: the corresponding nla_put() function (nla_put_flag()) takes
             only two args
 - __field4: the corresponding nla_put() function (nla_put_u64_64bit())
             takes four args

__field2 allows us to define __unspec_field for padding attribute.
__field4 allows us to update the definition of __u64_field: the pad
attribute should now be specified.

Note that this patch is only compile-tested.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v1 -> v2:
 rework the patch to handle all cases

 drivers/block/drbd/drbd_nl.c      | 40 +++++++++++++++++--------
 include/linux/drbd_genl.h         | 62 +++++++++++++++++++++------------------
 include/linux/genl_magic_func.h   | 41 ++++++++++++++++++++++++++
 include/linux/genl_magic_struct.h | 45 ++++++++++++++++++++++++++--
 4 files changed, 145 insertions(+), 43 deletions(-)

diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index 1fd1dccebb6b..93d873532195 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -3633,14 +3633,23 @@ static int nla_put_status_info(struct sk_buff *skb, struct drbd_device *device,
 		goto nla_put_failure;
 	if (nla_put_u32(skb, T_sib_reason, sib ? sib->sib_reason : SIB_GET_STATUS_REPLY) ||
 	    nla_put_u32(skb, T_current_state, device->state.i) ||
-	    nla_put_u64(skb, T_ed_uuid, device->ed_uuid) ||
-	    nla_put_u64(skb, T_capacity, drbd_get_capacity(device->this_bdev)) ||
-	    nla_put_u64(skb, T_send_cnt, device->send_cnt) ||
-	    nla_put_u64(skb, T_recv_cnt, device->recv_cnt) ||
-	    nla_put_u64(skb, T_read_cnt, device->read_cnt) ||
-	    nla_put_u64(skb, T_writ_cnt, device->writ_cnt) ||
-	    nla_put_u64(skb, T_al_writ_cnt, device->al_writ_cnt) ||
-	    nla_put_u64(skb, T_bm_writ_cnt, device->bm_writ_cnt) ||
+	    nla_put_u64_64bit(skb, T_ed_uuid, device->ed_uuid,
+			      T_state_info_pad) ||
+	    nla_put_u64_64bit(skb, T_capacity,
+			      drbd_get_capacity(device->this_bdev),
+			      T_state_info_pad) ||
+	    nla_put_u64_64bit(skb, T_send_cnt, device->send_cnt,
+			      T_state_info_pad) ||
+	    nla_put_u64_64bit(skb, T_recv_cnt, device->recv_cnt,
+			      T_state_info_pad) ||
+	    nla_put_u64_64bit(skb, T_read_cnt, device->read_cnt,
+			      T_state_info_pad) ||
+	    nla_put_u64_64bit(skb, T_writ_cnt, device->writ_cnt,
+			      T_state_info_pad) ||
+	    nla_put_u64_64bit(skb, T_al_writ_cnt, device->al_writ_cnt,
+			      T_state_info_pad) ||
+	    nla_put_u64_64bit(skb, T_bm_writ_cnt, device->bm_writ_cnt,
+			      T_state_info_pad) ||
 	    nla_put_u32(skb, T_ap_bio_cnt, atomic_read(&device->ap_bio_cnt)) ||
 	    nla_put_u32(skb, T_ap_pending_cnt, atomic_read(&device->ap_pending_cnt)) ||
 	    nla_put_u32(skb, T_rs_pending_cnt, atomic_read(&device->rs_pending_cnt)))
@@ -3657,13 +3666,20 @@ static int nla_put_status_info(struct sk_buff *skb, struct drbd_device *device,
 			goto nla_put_failure;
 
 		if (nla_put_u32(skb, T_disk_flags, device->ldev->md.flags) ||
-		    nla_put_u64(skb, T_bits_total, drbd_bm_bits(device)) ||
-		    nla_put_u64(skb, T_bits_oos, drbd_bm_total_weight(device)))
+		    nla_put_u64_64bit(skb, T_bits_total, drbd_bm_bits(device),
+				      T_state_info_pad) ||
+		    nla_put_u64_64bit(skb, T_bits_oos,
+				      drbd_bm_total_weight(device),
+				      T_state_info_pad))
 			goto nla_put_failure;
 		if (C_SYNC_SOURCE <= device->state.conn &&
 		    C_PAUSED_SYNC_T >= device->state.conn) {
-			if (nla_put_u64(skb, T_bits_rs_total, device->rs_total) ||
-			    nla_put_u64(skb, T_bits_rs_failed, device->rs_failed))
+			if (nla_put_u64_64bit(skb, T_bits_rs_total,
+					      device->rs_total,
+					      T_state_info_pad) ||
+			    nla_put_u64_64bit(skb, T_bits_rs_failed,
+					      device->rs_failed,
+					      T_state_info_pad))
 				goto nla_put_failure;
 		}
 	}
diff --git a/include/linux/drbd_genl.h b/include/linux/drbd_genl.h
index 2d0e5ad5de9d..f3b810089142 100644
--- a/include/linux/drbd_genl.h
+++ b/include/linux/drbd_genl.h
@@ -107,7 +107,7 @@ GENL_struct(DRBD_NLA_DISK_CONF, 3, disk_conf,
 	__s32_field(3, DRBD_F_REQUIRED | DRBD_F_INVARIANT,	meta_dev_idx)
 
 	/* use the resize command to try and change the disk_size */
-	__u64_field(4, DRBD_GENLA_F_MANDATORY | DRBD_F_INVARIANT,	disk_size)
+	__u64_field(4, DRBD_GENLA_F_MANDATORY | DRBD_F_INVARIANT,	disk_size, 24)
 	/* we could change the max_bio_bvecs,
 	 * but it won't propagate through the stack */
 	__u32_field(5, DRBD_GENLA_F_MANDATORY | DRBD_F_INVARIANT,	max_bio_bvecs)
@@ -132,6 +132,7 @@ GENL_struct(DRBD_NLA_DISK_CONF, 3, disk_conf,
 	__u32_field_def(21,	0 /* OPTIONAL */,       read_balancing, DRBD_READ_BALANCING_DEF)
 	/* 9: __u32_field_def(22,	DRBD_GENLA_F_MANDATORY,	unplug_watermark, DRBD_UNPLUG_WATERMARK_DEF) */
 	__flg_field_def(23,     0 /* OPTIONAL */,	al_updates, DRBD_AL_UPDATES_DEF)
+	__unspec_field(24,	0		,	disk_conf_pad)
 )
 
 GENL_struct(DRBD_NLA_RESOURCE_OPTS, 4, res_opts,
@@ -182,11 +183,12 @@ GENL_struct(DRBD_NLA_SET_ROLE_PARMS, 6, set_role_parms,
 )
 
 GENL_struct(DRBD_NLA_RESIZE_PARMS, 7, resize_parms,
-	__u64_field(1, DRBD_GENLA_F_MANDATORY,	resize_size)
+	__u64_field(1, DRBD_GENLA_F_MANDATORY,	resize_size, 6)
 	__flg_field(2, DRBD_GENLA_F_MANDATORY,	resize_force)
 	__flg_field(3, DRBD_GENLA_F_MANDATORY,	no_resync)
 	__u32_field_def(4, 0 /* OPTIONAL */, al_stripes, DRBD_AL_STRIPES_DEF)
 	__u32_field_def(5, 0 /* OPTIONAL */, al_stripe_size, DRBD_AL_STRIPE_SIZE_DEF)
+	__unspec_field(6, 0, resize_parms_pad)
 )
 
 GENL_struct(DRBD_NLA_STATE_INFO, 8, state_info,
@@ -194,8 +196,8 @@ GENL_struct(DRBD_NLA_STATE_INFO, 8, state_info,
 	 * if this is an event triggered broadcast. */
 	__u32_field(1, DRBD_GENLA_F_MANDATORY,	sib_reason)
 	__u32_field(2, DRBD_F_REQUIRED,	current_state)
-	__u64_field(3, DRBD_GENLA_F_MANDATORY,	capacity)
-	__u64_field(4, DRBD_GENLA_F_MANDATORY,	ed_uuid)
+	__u64_field(3, DRBD_GENLA_F_MANDATORY,	capacity, 24)
+	__u64_field(4, DRBD_GENLA_F_MANDATORY,	ed_uuid, 24)
 
 	/* These are for broadcast from after state change work.
 	 * prev_state and new_state are from the moment the state change took
@@ -208,30 +210,32 @@ GENL_struct(DRBD_NLA_STATE_INFO, 8, state_info,
 	/* if we have a local disk: */
 	__bin_field(7, DRBD_GENLA_F_MANDATORY,	uuids, (UI_SIZE*sizeof(__u64)))
 	__u32_field(8, DRBD_GENLA_F_MANDATORY,	disk_flags)
-	__u64_field(9, DRBD_GENLA_F_MANDATORY,	bits_total)
-	__u64_field(10, DRBD_GENLA_F_MANDATORY,	bits_oos)
+	__u64_field(9, DRBD_GENLA_F_MANDATORY,	bits_total, 24)
+	__u64_field(10, DRBD_GENLA_F_MANDATORY,	bits_oos, 24)
 	/* and in case resync or online verify is active */
-	__u64_field(11, DRBD_GENLA_F_MANDATORY,	bits_rs_total)
-	__u64_field(12, DRBD_GENLA_F_MANDATORY,	bits_rs_failed)
+	__u64_field(11, DRBD_GENLA_F_MANDATORY,	bits_rs_total, 24)
+	__u64_field(12, DRBD_GENLA_F_MANDATORY,	bits_rs_failed, 24)
 
 	/* for pre and post notifications of helper execution */
 	__str_field(13, DRBD_GENLA_F_MANDATORY,	helper, 32)
 	__u32_field(14, DRBD_GENLA_F_MANDATORY,	helper_exit_code)
 
-	__u64_field(15,                      0, send_cnt)
-	__u64_field(16,                      0, recv_cnt)
-	__u64_field(17,                      0, read_cnt)
-	__u64_field(18,                      0, writ_cnt)
-	__u64_field(19,                      0, al_writ_cnt)
-	__u64_field(20,                      0, bm_writ_cnt)
+	__u64_field(15,                      0, send_cnt, 24)
+	__u64_field(16,                      0, recv_cnt, 24)
+	__u64_field(17,                      0, read_cnt, 24)
+	__u64_field(18,                      0, writ_cnt, 24)
+	__u64_field(19,                      0, al_writ_cnt, 24)
+	__u64_field(20,                      0, bm_writ_cnt, 24)
 	__u32_field(21,                      0, ap_bio_cnt)
 	__u32_field(22,                      0, ap_pending_cnt)
 	__u32_field(23,                      0, rs_pending_cnt)
+	__unspec_field(24,                   0, state_info_pad)
 )
 
 GENL_struct(DRBD_NLA_START_OV_PARMS, 9, start_ov_parms,
-	__u64_field(1, DRBD_GENLA_F_MANDATORY,	ov_start_sector)
-	__u64_field(2, DRBD_GENLA_F_MANDATORY,	ov_stop_sector)
+	__u64_field(1, DRBD_GENLA_F_MANDATORY,	ov_start_sector, 3)
+	__u64_field(2, DRBD_GENLA_F_MANDATORY,	ov_stop_sector, 3)
+	__unspec_field(3, 0, ov_pad)
 )
 
 GENL_struct(DRBD_NLA_NEW_C_UUID_PARMS, 10, new_c_uuid_parms,
@@ -280,20 +284,21 @@ GENL_struct(DRBD_NLA_RESOURCE_STATISTICS, 19, resource_statistics,
 )
 
 GENL_struct(DRBD_NLA_DEVICE_STATISTICS, 20, device_statistics,
-	__u64_field(1, 0, dev_size)  /* (sectors) */
-	__u64_field(2, 0, dev_read)  /* (sectors) */
-	__u64_field(3, 0, dev_write)  /* (sectors) */
-	__u64_field(4, 0, dev_al_writes)  /* activity log writes (count) */
-	__u64_field(5, 0, dev_bm_writes)  /*  bitmap writes  (count) */
+	__u64_field(1, 0, dev_size, 15)  /* (sectors) */
+	__u64_field(2, 0, dev_read, 15)  /* (sectors) */
+	__u64_field(3, 0, dev_write, 15)  /* (sectors) */
+	__u64_field(4, 0, dev_al_writes, 15)  /* activity log writes (count) */
+	__u64_field(5, 0, dev_bm_writes, 15)  /*  bitmap writes  (count) */
 	__u32_field(6, 0, dev_upper_pending)  /* application requests in progress */
 	__u32_field(7, 0, dev_lower_pending)  /* backing device requests in progress */
 	__flg_field(8, 0, dev_upper_blocked)
 	__flg_field(9, 0, dev_lower_blocked)
 	__flg_field(10, 0, dev_al_suspended)  /* activity log suspended */
-	__u64_field(11, 0, dev_exposed_data_uuid)
-	__u64_field(12, 0, dev_current_uuid)
+	__u64_field(11, 0, dev_exposed_data_uuid, 15)
+	__u64_field(12, 0, dev_current_uuid, 15)
 	__u32_field(13, 0, dev_disk_flags)
 	__bin_field(14, 0, history_uuids, HISTORY_UUIDS * sizeof(__u64))
+	__unspec_field(15, 0, dev_pad)
 )
 
 GENL_struct(DRBD_NLA_CONNECTION_STATISTICS, 21, connection_statistics,
@@ -301,14 +306,15 @@ GENL_struct(DRBD_NLA_CONNECTION_STATISTICS, 21, connection_statistics,
 )
 
 GENL_struct(DRBD_NLA_PEER_DEVICE_STATISTICS, 22, peer_device_statistics,
-	__u64_field(1, 0, peer_dev_received)  /* sectors */
-	__u64_field(2, 0, peer_dev_sent)  /* sectors */
+	__u64_field(1, 0, peer_dev_received, 10)  /* sectors */
+	__u64_field(2, 0, peer_dev_sent, 10)  /* sectors */
 	__u32_field(3, 0, peer_dev_pending)  /* number of requests */
 	__u32_field(4, 0, peer_dev_unacked)  /* number of requests */
-	__u64_field(5, 0, peer_dev_out_of_sync)  /* sectors */
-	__u64_field(6, 0, peer_dev_resync_failed)  /* sectors */
-	__u64_field(7, 0, peer_dev_bitmap_uuid)
+	__u64_field(5, 0, peer_dev_out_of_sync, 10)  /* sectors */
+	__u64_field(6, 0, peer_dev_resync_failed, 10)  /* sectors */
+	__u64_field(7, 0, peer_dev_bitmap_uuid, 10)
 	__u32_field(9, 0, peer_dev_flags)
+	__unspec_field(10, 0, peer_dev_pad)
 )
 
 GENL_struct(DRBD_NLA_NOTIFICATION_HEADER, 23, drbd_notification_header,
diff --git a/include/linux/genl_magic_func.h b/include/linux/genl_magic_func.h
index 667c31101b8b..34b12f0836a5 100644
--- a/include/linux/genl_magic_func.h
+++ b/include/linux/genl_magic_func.h
@@ -35,6 +35,15 @@ static struct nla_policy s_name ## _nl_policy[] __read_mostly =		\
 		 __put, __is_signed)					\
 	[attr_nr] = { .type = nla_type },
 
+#undef __field2
+#define __field2 __field
+
+#undef __field4
+#define __field4(attr_nr, attr_flag, name, nla_type, _type, __get,	\
+		 __put, __is_signed, padattr)				\
+	__field(attr_nr, attr_flag, name, nla_type, _type, __get,       \
+		__put, __is_signed)
+
 #undef __array
 #define __array(attr_nr, attr_flag, name, nla_type, _type, maxlen,	\
 		__get, __put, __is_signed)				\
@@ -199,6 +208,15 @@ static int s_name ## _from_attrs_for_change(struct s_name *s,		\
 				s->name = __get(nla);			\
 			DPRINT_FIELD("<<", nla_type, name, s, nla))
 
+#undef __field2
+#define __field2 __field
+
+#undef __field4
+#define __field4(attr_nr, attr_flag, name, nla_type, _type, __get,	\
+		 __put, __is_signed, padattr)				\
+	__field(attr_nr, attr_flag, name, nla_type, _type, __get,       \
+		__put, __is_signed)
+
 /* validate_nla() already checked nla_len <= maxlen appropriately. */
 #undef __array
 #define __array(attr_nr, attr_flag, name, nla_type, type, maxlen,	\
@@ -362,6 +380,24 @@ static inline int s_name ## _to_unpriv_skb(struct sk_buff *skb,		\
 			goto nla_put_failure;				\
 	}
 
+#undef __field2
+#define __field2(attr_nr, attr_flag, name, nla_type, type, __get, __put,\
+		 __is_signed)						\
+	if (!exclude_sensitive || !((attr_flag) & DRBD_F_SENSITIVE)) {	\
+		DPRINT_FIELD(">>", nla_type, name, s, NULL);		\
+		if (__put(skb, attr_nr))				\
+			goto nla_put_failure;				\
+	}
+
+#undef __field4
+#define __field4(attr_nr, attr_flag, name, nla_type, type, __get, __put,\
+		 __is_signed, padattr)					\
+	if (!exclude_sensitive || !((attr_flag) & DRBD_F_SENSITIVE)) {	\
+		DPRINT_FIELD(">>", nla_type, name, s, NULL);		\
+		if (__put(skb, attr_nr, s->name, padattr))		\
+			goto nla_put_failure;				\
+	}
+
 #undef __array
 #define __array(attr_nr, attr_flag, name, nla_type, type, maxlen,	\
 		__get, __put, __is_signed)				\
@@ -381,6 +417,11 @@ static inline int s_name ## _to_unpriv_skb(struct sk_buff *skb,		\
 #undef __field
 #define __field(attr_nr, attr_flag, name, nla_type, type, __get, __put,	\
 		__is_signed)
+#undef __field2
+#define __field2 __field
+#undef __field4
+#define __field4(attr_nr, attr_flag, name, nla_type, type, __get, __put,\
+		__is_signed, padattr)
 #undef __array
 #define __array(attr_nr, attr_flag, name, nla_type, type, maxlen,	\
 		__get, __put, __is_signed)
diff --git a/include/linux/genl_magic_struct.h b/include/linux/genl_magic_struct.h
index eecd19b37001..768e63406540 100644
--- a/include/linux/genl_magic_struct.h
+++ b/include/linux/genl_magic_struct.h
@@ -66,6 +66,9 @@ extern void CONCAT_(GENL_MAGIC_FAMILY, _genl_unregister)(void);
 #define __flg_field(attr_nr, attr_flag, name) \
 	__field(attr_nr, attr_flag, name, NLA_U8, char, \
 			nla_get_u8, nla_put_u8, false)
+#define __unspec_field(attr_nr, attr_flag, name)	\
+	__field2(attr_nr, attr_flag, name, NLA_UNSPEC, unsigned char, \
+			nla_get_flag, nla_put_flag, false)
 #define __u8_field(attr_nr, attr_flag, name)	\
 	__field(attr_nr, attr_flag, name, NLA_U8, unsigned char, \
 			nla_get_u8, nla_put_u8, false)
@@ -78,9 +81,9 @@ extern void CONCAT_(GENL_MAGIC_FAMILY, _genl_unregister)(void);
 #define __s32_field(attr_nr, attr_flag, name)	\
 	__field(attr_nr, attr_flag, name, NLA_U32, __s32, \
 			nla_get_u32, nla_put_u32, true)
-#define __u64_field(attr_nr, attr_flag, name)	\
-	__field(attr_nr, attr_flag, name, NLA_U64, __u64, \
-			nla_get_u64, nla_put_u64, false)
+#define __u64_field(attr_nr, attr_flag, name, padattr)	\
+	__field4(attr_nr, attr_flag, name, NLA_U64, __u64, \
+			nla_get_u64, nla_put_u64_64bit, false, padattr)
 #define __str_field(attr_nr, attr_flag, name, maxlen) \
 	__array(attr_nr, attr_flag, name, NLA_NUL_STRING, char, maxlen, \
 			nla_strlcpy, nla_put, false)
@@ -156,6 +159,15 @@ enum {								\
 		__get, __put, __is_signed)			\
 	T_ ## name = (__u16)(attr_nr | ((attr_flag) & DRBD_GENLA_F_MANDATORY)),
 
+#undef __field2
+#define __field2 __field
+
+#undef __field4
+#define __field4(attr_nr, attr_flag, name, nla_type, _type, __get,	\
+		 __put, __is_signed, padattr)				\
+	__field(attr_nr, attr_flag, name, nla_type, _type, __get,       \
+		__put, __is_signed)
+
 #undef __array
 #define __array(attr_nr, attr_flag, name, nla_type, type,	\
 		maxlen, __get, __put, __is_signed)		\
@@ -222,6 +234,15 @@ static inline void ct_assert_unique_ ## s_name ## _attributes(void)	\
 		__is_signed)						\
 	case attr_nr:
 
+#undef __field2
+#define __field2 __field
+
+#undef __field4
+#define __field4(attr_nr, attr_flag, name, nla_type, _type, __get,	\
+		 __put, __is_signed, padattr)				\
+	__field(attr_nr, attr_flag, name, nla_type, _type, __get,       \
+		__put, __is_signed)
+
 #undef __array
 #define __array(attr_nr, attr_flag, name, nla_type, type, maxlen,	\
 		__get, __put, __is_signed)				\
@@ -246,6 +267,15 @@ struct s_name { s_fields };
 		__is_signed)						\
 	type name;
 
+#undef __field2
+#define __field2 __field
+
+#undef __field4
+#define __field4(attr_nr, attr_flag, name, nla_type, _type, __get,	\
+		 __put, __is_signed, padattr)				\
+	__field(attr_nr, attr_flag, name, nla_type, _type, __get,       \
+		__put, __is_signed)
+
 #undef __array
 #define __array(attr_nr, attr_flag, name, nla_type, type, maxlen,	\
 		__get, __put, __is_signed)				\
@@ -265,6 +295,15 @@ enum {									\
 		is_signed)						\
 	F_ ## name ## _IS_SIGNED = is_signed,
 
+#undef __field2
+#define __field2 __field
+
+#undef __field4
+#define __field4(attr_nr, attr_flag, name, nla_type, _type, __get,	\
+		 __put, __is_signed, padattr)				\
+	__field(attr_nr, attr_flag, name, nla_type, _type, __get,       \
+		__put, __is_signed)
+
 #undef __array
 #define __array(attr_nr, attr_flag, name, nla_type, type, maxlen,	\
 		__get, __put, is_signed)				\
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1393277 — Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

On Tue, May 03, 2016 at 11:39:18AM +0200, Nicolas Dichtel wrote:
> Two new handlers have been defined in genl_magic_ headers:
>  - __field2: the corresponding nla_put() function (nla_put_flag()) takes
>              only two args
>  - __field4: the corresponding nla_put() function (nla_put_u64_64bit())
>              takes four args
> 
> __field2 allows us to define __unspec_field for padding attribute.
> __field4 allows us to update the definition of __u64_field: the pad
> attribute should now be specified.

Please just NOT use an additional "field",
but always use 0 to pad.

Patch is much shorter as well, see below.

Attribute type "0" is not used,
and will never be of semantic value,
but always be ignored in the DRBD netlink family.

Whereas using some arbitrary value will be wrong,
and will needlessly break userland.

Thanks,

    Lars Ellenberg

diff --git a/include/linux/genl_magic_struct.h b/include/linux/genl_magic_struct.h
index eecd19b..6270a56 100644
--- a/include/linux/genl_magic_struct.h
+++ b/include/linux/genl_magic_struct.h
@@ -62,6 +62,11 @@ extern void CONCAT_(GENL_MAGIC_FAMILY, _genl_unregister)(void);
 
 /* MAGIC helpers							{{{2 */
 
+static inline int nla_put_u64_0pad(struct sk_buff *skb, int attrtype, u64 value)
+{
+	return nla_put_64bit(skb, attrtype, sizeof(u64), &value, 0);
+}
+
 /* possible field types */
 #define __flg_field(attr_nr, attr_flag, name) \
 	__field(attr_nr, attr_flag, name, NLA_U8, char, \
@@ -80,7 +85,7 @@ extern void CONCAT_(GENL_MAGIC_FAMILY, _genl_unregister)(void);
 			nla_get_u32, nla_put_u32, true)
 #define __u64_field(attr_nr, attr_flag, name)	\
 	__field(attr_nr, attr_flag, name, NLA_U64, __u64, \
-			nla_get_u64, nla_put_u64, false)
+			nla_get_u64, nla_put_u64_0pad, false)
 #define __str_field(attr_nr, attr_flag, name, maxlen) \
 	__array(attr_nr, attr_flag, name, NLA_NUL_STRING, char, maxlen, \
 			nla_strlcpy, nla_put, false)
diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index 1fd1dcc..206cc76 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -3633,14 +3633,14 @@ static int nla_put_status_info(struct sk_buff *skb, struct drbd_device *device,
 		goto nla_put_failure;
 	if (nla_put_u32(skb, T_sib_reason, sib ? sib->sib_reason : SIB_GET_STATUS_REPLY) ||
 	    nla_put_u32(skb, T_current_state, device->state.i) ||
-	    nla_put_u64(skb, T_ed_uuid, device->ed_uuid) ||
-	    nla_put_u64(skb, T_capacity, drbd_get_capacity(device->this_bdev)) ||
-	    nla_put_u64(skb, T_send_cnt, device->send_cnt) ||
-	    nla_put_u64(skb, T_recv_cnt, device->recv_cnt) ||
-	    nla_put_u64(skb, T_read_cnt, device->read_cnt) ||
-	    nla_put_u64(skb, T_writ_cnt, device->writ_cnt) ||
-	    nla_put_u64(skb, T_al_writ_cnt, device->al_writ_cnt) ||
-	    nla_put_u64(skb, T_bm_writ_cnt, device->bm_writ_cnt) ||
+	    nla_put_u64_0pad(skb, T_ed_uuid, device->ed_uuid) ||
+	    nla_put_u64_0pad(skb, T_capacity, drbd_get_capacity(device->this_bdev)) ||
+	    nla_put_u64_0pad(skb, T_send_cnt, device->send_cnt) ||
+	    nla_put_u64_0pad(skb, T_recv_cnt, device->recv_cnt) ||
+	    nla_put_u64_0pad(skb, T_read_cnt, device->read_cnt) ||
+	    nla_put_u64_0pad(skb, T_writ_cnt, device->writ_cnt) ||
+	    nla_put_u64_0pad(skb, T_al_writ_cnt, device->al_writ_cnt) ||
+	    nla_put_u64_0pad(skb, T_bm_writ_cnt, device->bm_writ_cnt) ||
 	    nla_put_u32(skb, T_ap_bio_cnt, atomic_read(&device->ap_bio_cnt)) ||
 	    nla_put_u32(skb, T_ap_pending_cnt, atomic_read(&device->ap_pending_cnt)) ||
 	    nla_put_u32(skb, T_rs_pending_cnt, atomic_read(&device->rs_pending_cnt)))
@@ -3657,13 +3657,13 @@ static int nla_put_status_info(struct sk_buff *skb, struct drbd_device *device,
 			goto nla_put_failure;
 
 		if (nla_put_u32(skb, T_disk_flags, device->ldev->md.flags) ||
-		    nla_put_u64(skb, T_bits_total, drbd_bm_bits(device)) ||
-		    nla_put_u64(skb, T_bits_oos, drbd_bm_total_weight(device)))
+		    nla_put_u64_0pad(skb, T_bits_total, drbd_bm_bits(device)) ||
+		    nla_put_u64_0pad(skb, T_bits_oos, drbd_bm_total_weight(device)))
 			goto nla_put_failure;
 		if (C_SYNC_SOURCE <= device->state.conn &&
 		    C_PAUSED_SYNC_T >= device->state.conn) {
-			if (nla_put_u64(skb, T_bits_rs_total, device->rs_total) ||
-			    nla_put_u64(skb, T_bits_rs_failed, device->rs_failed))
+			if (nla_put_u64_0pad(skb, T_bits_rs_total, device->rs_total) ||
+			    nla_put_u64_0pad(skb, T_bits_rs_failed, device->rs_failed))
 				goto nla_put_failure;
 		}
 	}

[toc] | [prev] | [next] | [standalone]

#1393369 — Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

Le 03/05/2016 12:06, Lars Ellenberg a écrit :
> On Tue, May 03, 2016 at 11:39:18AM +0200, Nicolas Dichtel wrote:
>> Two new handlers have been defined in genl_magic_ headers:
>>  - __field2: the corresponding nla_put() function (nla_put_flag()) takes
>>              only two args
>>  - __field4: the corresponding nla_put() function (nla_put_u64_64bit())
>>              takes four args
>>
>> __field2 allows us to define __unspec_field for padding attribute.
>> __field4 allows us to update the definition of __u64_field: the pad
>> attribute should now be specified.
> 
> Please just NOT use an additional "field",
> but always use 0 to pad.
> 
> Patch is much shorter as well, see below.
I don't think that the goal is to make the shortest patch...
But frankly, I don't care. The goal was to use the new interface in a proper
way, like every other subsystem.

> 
> Attribute type "0" is not used,
> and will never be of semantic value,
> but always be ignored in the DRBD netlink family.
> 
> Whereas using some arbitrary value will be wrong,
> and will needlessly break userland.
An application should always ignore unknown attribute, this is a golden rule.
Now if you know that this patch will break applications (which one exactly?), we
can use your proposal.


Regards,
Nicolas

[toc] | [prev] | [next] | [standalone]

#1393584 — Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

From: Lars Ellenberg <lars.ellenberg@linbit.com>
Date: Tue, 3 May 2016 12:06:44 +0200

> Please just NOT use an additional "field",
> but always use 0 to pad.

You can't, it doesn't work.

We are adding a new field to every netlink protocol family that has this
alignment problem.

[toc] | [prev] | [next] | [standalone]

#1394101 — Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

On Tue, May 03, 2016 at 12:05:56PM -0400, David Miller wrote:
> From: Lars Ellenberg <lars.ellenberg@linbit.com>
> Date: Tue, 3 May 2016 12:06:44 +0200
> 
> > Please just NOT use an additional "field",
> > but always use 0 to pad.
> 
> You can't, it doesn't work.

I did, and it *did* work.
At least, it appeared to.

I'm not talking about every user of netlink out there.
That I don't know. But specifically for DRBD netlink,
from what my experiments tell me, it works just fine.

> We are adding a new field to every netlink protocol family that has
> this alignment problem.

We don't have an "alignment problem" there, btw.
Last time I checked, we did work fine without this alignment magic,
we already take care of that, yes, even on affected architectures.

On Tue, May 03, 2016 at 12:06:52PM -0400, David Miller wrote:
> From: Lars Ellenberg <lars.ellenberg@linbit.com>
> Date: Tue, 3 May 2016 12:06:44 +0200
> 
> > Whereas using some arbitrary value will be wrong,
> > and will needlessly break userland.
> 
> It cannot break userland.

It can, if those tags have been used already.
There is DRBD out-of-tree as well,
it usually is ahead of in-tree DRBD.

But yes, I could obviously check and assign and reserve some
not-yet-used tag to all of them.

I don't see why, though, given that 0 (appearently) works fine.

Can you elaborate why and how that does not work?

	Lars

[toc] | [prev] | [next] | [standalone]

#1394259 — Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

Le 04/05/2016 11:05, Lars Ellenberg a écrit :
[snip]
> We don't have an "alignment problem" there, btw.
> Last time I checked, we did work fine without this alignment magic,
> we already take care of that, yes, even on affected architectures.
The code adds several consecutive u64 attributes. The nl attribute header is 4
bytes, thus the full attribute length is 12 bytes. If the first u64 is aligned
on 8 (nla_data()), the next one is not aligned on 8: it starts 12 bytes (8 (u64)
+ 4 (nl attr hdr)) after the previous u64.

[toc] | [prev] | [next] | [standalone]

#1394267 — Re: [Drbd-dev] [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

On Wed, May 04, 2016 at 02:49:00PM +0200, Nicolas Dichtel wrote:
> Le 04/05/2016 11:05, Lars Ellenberg a écrit :
> [snip]
> > We don't have an "alignment problem" there, btw.
> > Last time I checked, we did work fine without this alignment magic,
> > we already take care of that, yes, even on affected architectures.
> The code adds several consecutive u64 attributes. The nl attribute header is 4
> bytes, thus the full attribute length is 12 bytes. If the first u64 is aligned
> on 8 (nla_data()), the next one is not aligned on 8: it starts 12 bytes (8 (u64)
> + 4 (nl attr hdr)) after the previous u64.

Yes.  Which in our case is not a problem.
But I don't object to the padding per se,
if that is how things "should be".

I try to understand why you so much object to using 0 as pad.

    Lars

[toc] | [prev] | [next] | [standalone]

#1394338 — Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

On Wed, 2016-05-04 at 14:49 +0200, Nicolas Dichtel wrote:
> Le 04/05/2016 11:05, Lars Ellenberg a écrit :
> [snip]
> > We don't have an "alignment problem" there, btw.
> > Last time I checked, we did work fine without this alignment magic,
> > we already take care of that, yes, even on affected architectures.
> The code adds several consecutive u64 attributes. The nl attribute header is 4
> bytes, thus the full attribute length is 12 bytes. If the first u64 is aligned
> on 8 (nla_data()), the next one is not aligned on 8: it starts 12 bytes (8 (u64)
> + 4 (nl attr hdr)) after the previous u64.

As I mentioned earlier ( https://lkml.org/lkml/2016/4/22/706 ), if both
kernel and user land do not blindly use *(u64 *)ptr to put/get these
values, there was no issue.

kernel was fine, and most user land apps were fine as well.

Only for compound struct  like tcp_info this was nice to get alignment
for performance reason (it removed one memcpy(), or the use of
put_unaligned() helpers)

[toc] | [prev] | [next] | [standalone]

#1394544 — Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 04 May 2016 07:27:06 -0700

> kernel was fine, and most user land apps were fine as well.

Userland should really not have to deal with garbage like this.

And because it quietly works just fine on x86-64, nothing makes
sure that applications will universally get this right anyways.

Better to align things properly and magically all of these issues
simply disappear.

[toc] | [prev] | [next] | [standalone]

#1394559 — Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

On Wed, 2016-05-04 at 12:50 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 04 May 2016 07:27:06 -0700
> 
> > kernel was fine, and most user land apps were fine as well.
> 
> Userland should really not have to deal with garbage like this.
> 
> And because it quietly works just fine on x86-64, nothing makes
> sure that applications will universally get this right anyways.
> 
> Better to align things properly and magically all of these issues
> simply disappear.

Sure, but in practice we end up consuming 16 bytes (instead of 12) per
u64 attribute, only on some arches. 33 % space overhead.

So maybe some dumps will abort on those arches, while on x86 the size of
skb might be below some magic limit.

I guess this is fine, we do not break ABI in any way.

[toc] | [prev] | [next] | [standalone]

#1394537 — Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Wed, 4 May 2016 14:49:00 +0200

> Le 04/05/2016 11:05, Lars Ellenberg a écrit :
> [snip]
>> We don't have an "alignment problem" there, btw.
>> Last time I checked, we did work fine without this alignment magic,
>> we already take care of that, yes, even on affected architectures.
> The code adds several consecutive u64 attributes. The nl attribute header is 4
> bytes, thus the full attribute length is 12 bytes. If the first u64 is aligned
> on 8 (nla_data()), the next one is not aligned on 8: it starts 12 bytes (8 (u64)
> + 4 (nl attr hdr)) after the previous u64.

Right.

[toc] | [prev] | [next] | [standalone]

#1393586 — Re: [PATCH net-next v2] block/drbd: use nla_put_u64_64bit()

From: Lars Ellenberg <lars.ellenberg@linbit.com>
Date: Tue, 3 May 2016 12:06:44 +0200

> Whereas using some arbitrary value will be wrong,
> and will needlessly break userland.

It cannot break userland.

A fundamental property of netlink is that all code must silently
ignore netlink attributes it does not understand.

This is why netlink is easily extensible.

If code isn't doing that, it is broken and must be fixed.

[toc] | [prev] | [next] | [standalone]

#1396891 — [PATCH net-next v3] block/drbd: align properly u64 in nl messages

The attribute 0 is never used in drbd, so let's use it as pad attribute
in netlink messages. This minimizes the patch.

Note that this patch is only compile-tested.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com>
---

v2 -> v3:
  use 0 as padattr instead of adding new attributes

v1 -> v2:
 rework the patch to handle all cases

Maybe prefixing genl_magic_func.h and genl_magic_struct.h by 'drbd_'
could be interesting so that new module won't use it. What is your
opinion?

 drivers/block/drbd/drbd_nl.c      | 28 ++++++++++++++++------------
 include/linux/genl_magic_struct.h |  7 ++++++-
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index 1fd1dccebb6b..0bac9c8246bc 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -3633,14 +3633,15 @@ static int nla_put_status_info(struct sk_buff *skb, struct drbd_device *device,
 		goto nla_put_failure;
 	if (nla_put_u32(skb, T_sib_reason, sib ? sib->sib_reason : SIB_GET_STATUS_REPLY) ||
 	    nla_put_u32(skb, T_current_state, device->state.i) ||
-	    nla_put_u64(skb, T_ed_uuid, device->ed_uuid) ||
-	    nla_put_u64(skb, T_capacity, drbd_get_capacity(device->this_bdev)) ||
-	    nla_put_u64(skb, T_send_cnt, device->send_cnt) ||
-	    nla_put_u64(skb, T_recv_cnt, device->recv_cnt) ||
-	    nla_put_u64(skb, T_read_cnt, device->read_cnt) ||
-	    nla_put_u64(skb, T_writ_cnt, device->writ_cnt) ||
-	    nla_put_u64(skb, T_al_writ_cnt, device->al_writ_cnt) ||
-	    nla_put_u64(skb, T_bm_writ_cnt, device->bm_writ_cnt) ||
+	    nla_put_u64_0pad(skb, T_ed_uuid, device->ed_uuid) ||
+	    nla_put_u64_0pad(skb, T_capacity,
+			     drbd_get_capacity(device->this_bdev)) ||
+	    nla_put_u64_0pad(skb, T_send_cnt, device->send_cnt) ||
+	    nla_put_u64_0pad(skb, T_recv_cnt, device->recv_cnt) ||
+	    nla_put_u64_0pad(skb, T_read_cnt, device->read_cnt) ||
+	    nla_put_u64_0pad(skb, T_writ_cnt, device->writ_cnt) ||
+	    nla_put_u64_0pad(skb, T_al_writ_cnt, device->al_writ_cnt) ||
+	    nla_put_u64_0pad(skb, T_bm_writ_cnt, device->bm_writ_cnt) ||
 	    nla_put_u32(skb, T_ap_bio_cnt, atomic_read(&device->ap_bio_cnt)) ||
 	    nla_put_u32(skb, T_ap_pending_cnt, atomic_read(&device->ap_pending_cnt)) ||
 	    nla_put_u32(skb, T_rs_pending_cnt, atomic_read(&device->rs_pending_cnt)))
@@ -3657,13 +3658,16 @@ static int nla_put_status_info(struct sk_buff *skb, struct drbd_device *device,
 			goto nla_put_failure;
 
 		if (nla_put_u32(skb, T_disk_flags, device->ldev->md.flags) ||
-		    nla_put_u64(skb, T_bits_total, drbd_bm_bits(device)) ||
-		    nla_put_u64(skb, T_bits_oos, drbd_bm_total_weight(device)))
+		    nla_put_u64_0pad(skb, T_bits_total, drbd_bm_bits(device)) ||
+		    nla_put_u64_0pad(skb, T_bits_oos,
+				     drbd_bm_total_weight(device)))
 			goto nla_put_failure;
 		if (C_SYNC_SOURCE <= device->state.conn &&
 		    C_PAUSED_SYNC_T >= device->state.conn) {
-			if (nla_put_u64(skb, T_bits_rs_total, device->rs_total) ||
-			    nla_put_u64(skb, T_bits_rs_failed, device->rs_failed))
+			if (nla_put_u64_0pad(skb, T_bits_rs_total,
+					     device->rs_total) ||
+			    nla_put_u64_0pad(skb, T_bits_rs_failed,
+					     device->rs_failed))
 				goto nla_put_failure;
 		}
 	}
diff --git a/include/linux/genl_magic_struct.h b/include/linux/genl_magic_struct.h
index eecd19b37001..6270a56e5edc 100644
--- a/include/linux/genl_magic_struct.h
+++ b/include/linux/genl_magic_struct.h
@@ -62,6 +62,11 @@ extern void CONCAT_(GENL_MAGIC_FAMILY, _genl_unregister)(void);
 
 /* MAGIC helpers							{{{2 */
 
+static inline int nla_put_u64_0pad(struct sk_buff *skb, int attrtype, u64 value)
+{
+	return nla_put_64bit(skb, attrtype, sizeof(u64), &value, 0);
+}
+
 /* possible field types */
 #define __flg_field(attr_nr, attr_flag, name) \
 	__field(attr_nr, attr_flag, name, NLA_U8, char, \
@@ -80,7 +85,7 @@ extern void CONCAT_(GENL_MAGIC_FAMILY, _genl_unregister)(void);
 			nla_get_u32, nla_put_u32, true)
 #define __u64_field(attr_nr, attr_flag, name)	\
 	__field(attr_nr, attr_flag, name, NLA_U64, __u64, \
-			nla_get_u64, nla_put_u64, false)
+			nla_get_u64, nla_put_u64_0pad, false)
 #define __str_field(attr_nr, attr_flag, name, maxlen) \
 	__array(attr_nr, attr_flag, name, NLA_NUL_STRING, char, maxlen, \
 			nla_strlcpy, nla_put, false)
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1397039 — Re: [PATCH net-next v3] block/drbd: align properly u64 in nl messages

On Mon, May 09, 2016 at 11:40:20AM +0200, Nicolas Dichtel wrote:
> The attribute 0 is never used in drbd, so let's use it as pad attribute
> in netlink messages. This minimizes the patch.
> 
> Note that this patch is only compile-tested.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com>
> ---
> 
> v2 -> v3:
>   use 0 as padattr instead of adding new attributes

Thanks.

> v1 -> v2:
>  rework the patch to handle all cases
> 
> Maybe prefixing genl_magic_func.h and genl_magic_struct.h by 'drbd_'
> could be interesting so that new module won't use it. What is your
> opinion?

This was supposed to not be DRBD specific.  But it might even still
need some massaging before it was truly generic. And obviously,
it does not meet the taste of genetlink folks, to say the least :(

I don't care either way.

    Lars Ellenberg

[toc] | [prev] | [next] | [standalone]

#1397883 — Re: [PATCH net-next v3] block/drbd: align properly u64 in nl messages

Le 09/05/2016 15:15, Lars Ellenberg a écrit :
> On Mon, May 09, 2016 at 11:40:20AM +0200, Nicolas Dichtel wrote:
[snip]
>> Maybe prefixing genl_magic_func.h and genl_magic_struct.h by 'drbd_'
>> could be interesting so that new module won't use it. What is your
>> opinion?
> 
> This was supposed to not be DRBD specific.  But it might even still
> need some massaging before it was truly generic. And obviously,
> it does not meet the taste of genetlink folks, to say the least :(
Yes, this file is not generic and netlink APIs are never defined like this.
These tons of macro complexifies the code too much. It's overengineering for
what purpose?
Small examples:
 - the drbd netlink API is not exported via uapi (I wonder how apps using this
   API get it)
 - v2 of the patch is nacked because adding a new attribute may break existing
   apps (in networking code, a lot of new attributes are added in each version)
 - it's not possible to grep to show the definition of an attribute ('git grep
   -w T_bits_total' returns only 1 line)


Regards,
Nicolas

[toc] | [prev] | [next] | [standalone]

#1397905 — Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in nl messages

On Tue, May 10, 2016 at 11:09:53AM +0200, Nicolas Dichtel wrote:
> Le 09/05/2016 15:15, Lars Ellenberg a écrit :
> > On Mon, May 09, 2016 at 11:40:20AM +0200, Nicolas Dichtel wrote:
> [snip]
> >> Maybe prefixing genl_magic_func.h and genl_magic_struct.h by 'drbd_'
> >> could be interesting so that new module won't use it. What is your
> >> opinion?
> > 
> > This was supposed to not be DRBD specific.  But it might even still
> > need some massaging before it was truly generic. And obviously,
> > it does not meet the taste of genetlink folks, to say the least :(
> Yes, this file is not generic and netlink APIs are never defined like this.
> These tons of macro complexifies the code too much. It's overengineering for
> what purpose?

If we introduce a new config option,
we have to add it to the config scanner (one line),
define min, max, default and scale (four short defines),
and add it to the netlink definition here (one line).
Done, rest of the code is generated,
both on the kernel side,
and on the drbd-utils side used to talk to the kernel.
We found that to be very convenient.

> Small examples:
>  - the drbd netlink API is not exported via uapi (I wonder how apps using this
>    API get it)

There used to be a time where there was no "uapi".
(I wonder how apps ever worked back then).

>  - v2 of the patch is nacked because adding a new attribute may break existing

No.

But because the "new" attributes you chose have not been new,
but already used (though not yet merged back into mainline yet).
(Which you did not realize, and had no obvious way of knowing.
 Could have been fixed.).

And because your patch introduced useless new members to the structs.
(Could also have been fixed).

And because I did not see any use defining that many new "padding attributes"
for no reason, where the obvious (to me) choice was to use 0, and you
did not even try to explain why that would have been a bad choice.

>    apps (in networking code, a lot of new attributes are added in each version)


>  - it's not possible to grep to show the definition of an attribute ('git grep
>    -w T_bits_total' returns only 1 line)

Opencoded, it would return 2.

 ;-)

Is this going somewhere?

Cheers,

    Lars

[toc] | [prev] | [next] | [standalone]

#1397953 — Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in nl messages

Le 10/05/2016 11:40, Lars Ellenberg a écrit :
> On Tue, May 10, 2016 at 11:09:53AM +0200, Nicolas Dichtel wrote:
>> Le 09/05/2016 15:15, Lars Ellenberg a écrit :
>>> On Mon, May 09, 2016 at 11:40:20AM +0200, Nicolas Dichtel wrote:
>> [snip]
>>>> Maybe prefixing genl_magic_func.h and genl_magic_struct.h by 'drbd_'
>>>> could be interesting so that new module won't use it. What is your
>>>> opinion?
>>>
>>> This was supposed to not be DRBD specific.  But it might even still
>>> need some massaging before it was truly generic. And obviously,
>>> it does not meet the taste of genetlink folks, to say the least :(
>> Yes, this file is not generic and netlink APIs are never defined like this.
>> These tons of macro complexifies the code too much. It's overengineering for
>> what purpose?
> 
> If we introduce a new config option,
> we have to add it to the config scanner (one line),
> define min, max, default and scale (four short defines),
> and add it to the netlink definition here (one line).
> Done, rest of the code is generated,
> both on the kernel side,
> and on the drbd-utils side used to talk to the kernel.
> We found that to be very convenient.
Ok.

> 
>> Small examples:
>>  - the drbd netlink API is not exported via uapi (I wonder how apps using this
>>    API get it)
> 
> There used to be a time where there was no "uapi".
> (I wonder how apps ever worked back then).
At that time, include/linux/ was exported ;-)

> 
>>  - v2 of the patch is nacked because adding a new attribute may break existing
> 
> No.
> 
> But because the "new" attributes you chose have not been new,
> but already used (though not yet merged back into mainline yet).
> (Which you did not realize, and had no obvious way of knowing.
>  Could have been fixed.).
Ok.

> 
> And because your patch introduced useless new members to the structs.
> (Could also have been fixed).
> 
> And because I did not see any use defining that many new "padding attributes"
> for no reason, where the obvious (to me) choice was to use 0, and you
> did not even try to explain why that would have been a bad choice.
Because some nl APIs were wrongly use 0 as a valid attribute we make the choice
of always adding a new attribute for padding to be sure to not break existing API.
And yes, in drdb it does not seem to be the case.

> Is this going somewhere?
I'm just trying to understand things.


Regards,
Nicolas

[toc] | [prev] | [next] | [standalone]

#1398256 — Re: [Drbd-dev] [PATCH net-next v3] block/drbd: align properly u64 in nl messages

From: Lars Ellenberg <lars.ellenberg@linbit.com>
Date: Tue, 10 May 2016 11:40:23 +0200

> If we introduce a new config option,
> we have to add it to the config scanner (one line),
> define min, max, default and scale (four short defines),
> and add it to the netlink definition here (one line).
> Done, rest of the code is generated,
> both on the kernel side,
> and on the drbd-utils side used to talk to the kernel.
> We found that to be very convenient.

But it entirely misses the core design point of netlink.

Sender and receive _DO NOT_ need to coordinate at all.  That's the
whole point.  So tightly coupling such coordination is going to run
you into all kinds of problems.

When implemented properly, the sender can emit whatever attributes it
knows about and can generate, and the receive scans the attributes one
by one and picks out the ones it understands and processes them.

If you go against this model then you have no clean way to extend
things whilst allowing existing software to continue working.

If the drbd stuff had been posting to the networking list, we really
would have screamed loudly about these auto-generates structs and
macros and whatnot.

Anyways, back to the topic, can you please just relent and come to some
kind of agreement about the fix for this alignment bug?  This is taking
a very long time and patches are just rotting in patchwork with no
resolution.

Thanks.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web