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

[Mark Harvey]

On 6/28/07, FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp> wrote:
> 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.

OK - same page now...

The SSC module will be pulling this in as well as the current SMC module.

Hence I separated into its own header file.

>
>
> > > > 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.
>

Starting to push my boundary of C knowledge here. But I think I
understand what you are trying to tell me. Its the only way to
learn...

I need some time to look at this (to make sure I do understand it)..

I'll look at this (this evening) and see what I come up with..

Cheers
Mark