Re: [ndctl PATCH 1/7] libcxl: add GET_PARTITION_INFO mailbox command and accessors

[Ira Weiny <ira.weiny@intel.com>]

On Thu, Jan 06, 2022 at 01:21:45PM -0800, Dan Williams wrote:
> On Thu, Jan 6, 2022 at 12:19 PM Ira Weiny <ira.weiny@intel.com> wrote:
> >
> > On Mon, Jan 03, 2022 at 12:16:12PM -0800, Schofield, Alison wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > >
> > > diff --git a/cxl/libcxl.h b/cxl/libcxl.h
> > > index 89d35ba..7cf9061 100644
> > > --- a/cxl/libcxl.h
> > > +++ b/cxl/libcxl.h
> > > @@ -109,6 +109,11 @@ ssize_t cxl_cmd_read_label_get_payload(struct cxl_cmd *cmd, void *buf,
> > >               unsigned int length);
> > >  struct cxl_cmd *cxl_cmd_new_write_label(struct cxl_memdev *memdev,
> > >               void *buf, unsigned int offset, unsigned int length);
> > > +struct cxl_cmd *cxl_cmd_new_get_partition_info(struct cxl_memdev *memdev);
> >
> > why 'new'?  Why not:
> >
> > cxl_cmd_get_partition_info()
> >
> > ?
> 
> The "new" is the naming convention inherited from ndctl indicating the
> allocation of a new command object. The motivation is to have a verb /
> action in all of the APIs.

Yea my bad.  I realized that later on.  Sorry.

> 
> >
> > > +unsigned long long cxl_cmd_get_partition_info_get_active_volatile_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_active_persistent_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_next_volatile_cap(struct cxl_cmd *cmd);
> > > +unsigned long long cxl_cmd_get_partition_info_get_next_persistent_cap(struct cxl_cmd *cmd);
> >
> > These are pretty long function names.  :-/
> 
> If you think those are long, how about:
> 
> cxl_cmd_health_info_get_media_powerloss_persistence_loss
> 
> The motivation here is to keep data structure layouts hidden behind
> APIs to ease the maintenance of binary compatibility from one library
> release and specification release to the next. The side effect though
> is some long function names in places.

Sure.  I'm ok with that.

Ira

> 
> > I also wonder if the conversion to bytes should be reflected in the function
> > name.  Because returning 'cap' might imply to someone they are getting the raw
> > data field.
> 
> Makes sense.