Re: [RFC PATCH 13/20] ceph: add comments to metadata structures in msgr.h

[Max Kellermann <max@blarg.de>]

On 2025/09/05 22:01, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> Claude AI generated comments for CephFS metadata structure
> declarations in include/linux/ceph/*.h. These comments
> have been reviewed, checked, and corrected.

After looking at several of your patches, I doubt the value of this
effort.  This is mostly just redundant noise being added.

>  struct ceph_entity_addr {
> +	/* Address type identifier */
>  	__le32 type;  /* CEPH_ENTITY_ADDR_TYPE_* */

The old comment, which you (accidently?) left in there, is better than
your new comment.  It specifies what values are possible.

"Address type identifier" means nothing to me, no more than "type" of
"ceph_entity_addr".

> -	__le32 nonce;  /* unique id for process (e.g. pid) */
> +	/* Unique process identifier (typically PID) */
> +	__le32 nonce;

No improvement here.  Just rephrased to be a bit more verbose without
adding any information.

> +	/* Socket address (IPv4/IPv6) */
>  	struct sockaddr_storage in_addr;

Surprise - a "struct sockaddr_storage" is a socket address?  Does this
need to be explained?

What is the value of pointing out that it can be IPv4 or IPv6?  You
asked Claude to generate tokens, and so it did.  But do these tokens
add any value?  I don't think so.

>  struct ceph_entity_inst {
> +	/* Logical entity name (type + number) */
>  	struct ceph_entity_name name;

I don't understand this comment.  Okay, so the entity name is an
entity name, but what does it mean for a name to be "logical"?  What
is this "type" and "number" and how do you add them?

> +	/* Network address for this entity */
>  	struct ceph_entity_addr addr;

Oh well.  The address is an address.

> +	/* Wire protocol version */
>  	__le32 protocol_version;

The protocol version is one for the wire.  A-ha.

> +	/* Connection flags (lossy, etc.) */
>  	__u8  flags;         /* CEPH_MSG_CONNECT_* */

The old comment looked fine.  I can "git grep CEPH_MSG_CONNECT_" and
see possible values.

The new comment says these flags are for the connection.  Okay.  And
the flags can be lossy, can't they?  No, one of the possibly flags is
just "CEPH_MSG_CONNECT_LOSSY" and Claude confusingly rephrased this to
"lossy".  And "etc.", but what is "etc."?  Nothing, there are no other
flags.  The "etc." is just noise.

What would have been valuable pointing out is that this is a bit mask
(but this can be guessed easily).

Just leave the old comment, it's fine and enough.

>  struct ceph_msg_connect_reply {
> +	/* Reply tag (ready, retry, error, etc.) */
>  	__u8 tag;

How does "ready", "retry" etc. fit into a __u8?  Claude unhelpfully
omitted technical details by rephrasing the macro names.


(That's all I'm going to comment on now.  I could go on for hours, but
I feel this is a waste of time.)