[Stgt-devel] [Patch 3/3] Re-work of SMC support

[FUJITA Tomonori]

From: "Mark Harvey" <markh794 at gmail.com>
Subject: Re: [Stgt-devel] [Patch 3/3] Re-work of SMC support
Date: Thu, 28 Jun 2007 10:20:16 +1000

> On 6/27/07, FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp> wrote:
> > From: Mark Harvey <markh794 at gmail.com>
> > Subject: [Stgt-devel] [Patch 3/3] Re-work of SMC support
> > Date: Sat, 23 Jun 2007 18:14:04 +1000
> >
> > > diff --git a/usr/media.h b/usr/media.h
> > > new file mode 100644
> > > index 0000000..a04a27c
> > > --- /dev/null
> > > +++ b/usr/media.h
> > > @@ -0,0 +1,30 @@
> > > +/*
> > > + * Copyright (C) 2007 Mark Harvey markh794 at gmail dot com
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; version 2 of the License.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > + * along with this program; if not, write to the Free Software
> > > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > > + */
> > > +#ifndef _MEDIA_H_
> > > +#define _MEDIA_H_
> > > +
> > > +enum c_type {        /* Cartridge Types - Ref: smc3r06 - Table 20, page 37 */
> > > +     CART_UNSPECIFIED,
> > > +     CART_DATA,
> > > +     CART_CLEAN,
> > > +     CART_DIAGNOSTICS,
> > > +     CART_WORM,
> > > +     CART_MICROCODE,
> > > +};
> > > +
> > > +#endif /* _MEDIA_H_ */
> >
> > Why do you put this in smc.c?
> 
> Slots within the SMC device can contain different media type..
> 
> At the moment, this is not used by the SMC device but I included for
> when this support is added.
> 
> Should I remove this until I get around to coding this portion of the code ?

I'm not sure whether we are on the same page.

What I wanted to say is why you need to put enum c_type to media.h
instead of smc.c.


> > > diff --git a/usr/mmc.c b/usr/mmc.c
> > > index 891cb60..0672877 100644
> > > --- a/usr/mmc.c
> > > +++ b/usr/mmc.c
> > > @@ -38,6 +38,7 @@
> > >  #include "util.h"
> > >  #include "tgtd.h"
> > >  #include "target.h"
> > > +#include "tgtadm_error.h"
> > >  #include "driver.h"
> > >  #include "scsi.h"
> > >  #include "spc.h"
> > > @@ -174,10 +175,26 @@ static int mmc_lu_init(struct scsi_lu *lu)
> > >       return 0;
> > >  }
> > >
> > > +static int mmc_lu_config(struct scsi_lu *lu, char * params)
> > > +{
> > > +     char    * mmc_params;
> > > +     int     ret;
> > > +
> > > +     mmc_params = malloc(strlen(params) + 1);
> > > +     if (!mmc_params)
> > > +             return -ENOMEM;
> > > +     mmc_params[0] = '\0';
> > > +     ret = spc_lu_config(lu, params, mmc_params);
> > > +     if (strlen(mmc_params)) /* We should not have any params left over */
> > > +             ret = TGTADM_INVALID_REQUEST;
> > > +     free(mmc_params);
> > > +     return ret;
> > > +}
> >
> > strdup?
> 
> I'm still trying to come up with a 'better' way of doing this.
> 
> In an attempt to move all common parameter parsing within one common
> function and still be able to process any module unique params
> afterwards :
> 
>      ret = spc_lu_config(lu, params, mmc_params);
> 
>  - Original string of params passed in 'params', 'mmc_params' is a null string.
>  - The spc_lu_config() copies any parameters it can not process in the
> *mmc_params.
>  - It is then up the the xxx_lu_config() calling function to process
> any left over params (in mmc_params in this case) not handled by
> spc_lu_config().
>  - If all params are processed in spc_lu_config(), then the
> *mmc_params will contain an empty string.
> 
> I thought it might be a little more obvious to have the malloc() /
> free() within the same function rather then have spc_lu_config do the
> malloc() and then the caller free the space afterwards.
> 
> The smc_lu_config() is the only module at the moment that needs the
> extra options.
> 
> I expect the ssc_lu_config() to also require extra configuration
> options. Although I have not started coding that yet and have not
> fully thought thru what may be required.
> 
> 
> 
> The other option would be to move the spc_lu_config() into each module
> and not share the common param handling.

How about:

typedef int (match_fn_t) (struct scsi_lu *);

int spc_lu_config(struct scsi_lu *lu, char *params, match_fn_t *fn,
match_table_t *t)
{
}

spc_lu_config calls fn with t (which device code like smc passes) for
non-standard params.