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

[Mark Harvey]

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 ?


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

Cheers
Mark