Re: [PATCH v3] pkl: Remove global state from IOS

[Mohammad-Reza Nabipoor]

Hi, Jose.

On Fri, Dec 24, 2021 at 12:30:41PM +0100, Jose E. Marchesi wrote:
> 
> Hi Mohammad.
> 
> Thanks for the patch.  Please see comments below.
> 
> > diff --git a/libpoke/ios-dev-sub.c b/libpoke/ios-dev-sub.c
> > index f6646dab..7b798c0d 100644
> > --- a/libpoke/ios-dev-sub.c
> > +++ b/libpoke/ios-dev-sub.c
> > @@ -32,6 +32,7 @@
> >  
> >  struct ios_dev_sub
> >  {
> > +  ios_context ictx;
> 
> Please use a more significant name, like ios_ctxt.  Ditto in other
> places.


You mean s/ictx/ios_ctx/ everywhere?


> 
> > diff --git a/libpoke/ios.c b/libpoke/ios.c
> > index 6bc5bd12..3bce3c93 100644
> > --- a/libpoke/ios.c
> > +++ b/libpoke/ios.c
> > @@ -78,15 +78,6 @@ struct ios
> >    struct ios *next;
> >  };
> >  
> > -/* Next available IOS id.  */
> > -
> > -static int ios_next_id = 0;
> > -
> > -/* List of IO spaces, and pointer to the current one.  */
> > -
> > -static struct ios *io_list;
> > -static struct ios *cur_io;
> > -
> >  /* The available backends are implemented in their own files, and
> >     provide the following interfaces.  */
> >  
> > @@ -102,83 +93,116 @@ extern struct ios_dev_if ios_dev_proc; /* 
> > ios-dev-proc.c */
> >  #endif
> >  extern struct ios_dev_if ios_dev_sub; /* ios-dev-sub.c */
> >  
> > -static struct ios_dev_if *ios_dev_ifs[] =
> > -  {
> > -   NULL, /* Optional foreign IOD.  */
> > -   &ios_dev_zero,
> > -   &ios_dev_mem,
> > -   &ios_dev_stream,
> > +enum
> > +{
> > +  IOS_DEV_FOREIGN, /* Foreign must be first */
> > +  IOS_DEV_ZERO,
> > +  IOS_DEV_MEM,
> > +  IOS_DEV_STREAM,
> > +  IOS_DEV_NBD,
> > +  IOS_DEV_PROC,
> > +  IOS_DEV_SUB,
> > +  IOS_DEV_FILE, /* File must be last */
> > +
> > +  IOS_DEV_COUNT
> > +};
> > +
> > +struct ios_context
> > +{
> > +  int next_id;         /* Next available IOS id.  */
> > +  struct ios *io_list; /* List of all IO spaces.  */
> > +  struct ios *cur_io;  /* Pointer to the current IOS.  */
> > +  struct ios_dev_if dev_ifs[IOS_DEV_COUNT]; /* IO devices.  */
> > +  int dev_ifs_valid_p[IOS_DEV_COUNT];       /* Validity of IO devices.  */
> > +};
> > +
> > +ios_context
> > +ios_init (void)
> > +{
> > +  ios_context ictx = calloc (1, sizeof (struct ios_context));
> > +
> > +  if (!ictx)
> > +    return NULL;
> > +
> > +#define DEV_IF(idx, val)                                                   
> >    \
> > +  do                                                                       
> >    \
> > +    {                                                                      
> >    \
> > +      memcpy (&ictx->dev_ifs[IOS_DEV_##idx], &ios_dev_##val,               
> >    \
> > +              sizeof (struct ios_dev_if));                                 
> >    \
> > +      ictx->dev_ifs_valid_p[IOS_DEV_##idx] = 1;                            
> >    \
> > +    }                                                                      
> >    \
> > +  while (0)
> 
> Hmm, I don't like the fact we are copying the ios_dev_if structures
> around.  Looks too complicated.  Is this because they hold `data'
> pointers per ios context?
> 

Yes, I don't like it either! But because of `data` pointers (which are
specific to instances of `libpoke`), we have to.

Do you have any better idea?
(I thought about removing `data` from the `struct ios_dev_if`, and keep it
separate, but this means `struct pk_iod_if` and `struct ios_dev_if` will not
share the same physical layout).