[Stgt-devel] [Patch 1/8 ] Setup .device_init, device_shutdown & device_config to the device template.

[Mark Harvey]

Thanks for the feedback.

My aim is to produce code of sufficient quality to be integrated with
this project.

This is my first attempt at submitting code to any project, so I
appreciate your feedback & patience.

On 5/30/07, FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp> wrote:
> Thanks for the patches. Sorry for the delay.
>
> > diff --git a/usr/mmc.c b/usr/mmc.c
> > index e9cc479..d53d0f3 100644
> > --- a/usr/mmc.c
> > +++ b/usr/mmc.c
> > @@ -121,10 +121,24 @@ static int mmc_read_capacity(int host_no, struct scsi_cmd *cmd)
> >       return SAM_STAT_GOOD;
> >  }
> >
>
> trailing spaces

OK - "trailing spaces" is a subject I'm unsure about.

Looking at the code, a 'cat -E mmc.c' shows no trailing spaces...

The "trailing spaces" appears in various mailing lists many times (not
just this one), so any tips, references to this subject is most
welcome.
  ============================
[snip]
        size = cmd->dev->size >> MMC_BLK_SHIFT;$
$
        data[0] = (size >> 32) ?$
                __cpu_to_be32(0xffffffff) : __cpu_to_be32(size - 1);$
        data[1] = __cpu_to_be32(1U << MMC_BLK_SHIFT);$
        cmd->len = 8;$
$
        return SAM_STAT_GOOD;$
}$
$
  ============================
>
>
> > +static int device_mmc_init(struct scsi_lu *lu)
> > +{
> > +     lu->d_sense = 1;
> > +     return 0;
> > +}
> > +
> > +static int device_mmc_shutdown(struct scsi_lu *lu)
> > +{
> > +     return 0;
> > +}
>
> In general, the kernel uses init/exit pair, I guess. So I prefer it.

Easily changed..

>
>
> >  struct device_type_template mmc_template = {
> >       .type   = TYPE_ROM,
> >       .name   = "cdrom/dvd",
> >       .pid    = "VIRTUAL-CDROM",
> > +     .device_init = device_mmc_init,
> > +     .device_shutdown = device_mmc_shutdown,
> > +     .device_config = spc_device_config,
>
> lu_init/lu_exit/lu_config looks better?

OK.

>
> Yeah, it would be better to rename device_* functions too. I take care
> about that.

Yeah, I saw this and I have made it consistent


>
> >       .ops    = {
> >               {spc_test_unit,},
> >               {spc_illegal_op,},
> > diff --git a/usr/osd.c b/usr/osd.c
> > index 46bf0a0..64e7b2a 100644
> > --- a/usr/osd.c
> > +++ b/usr/osd.c
> > @@ -145,15 +145,22 @@ static int osd_varlen_cdb(int host_no, struct scsi_cmd *cmd)
> >       return cmd->c_target->bst->bs_cmd_submit(cmd);
> >  }
> >
>
> trailing spaces
>
> All the patches seem to have tons of trailing spaces.

Request for assistance in identifying why these are occurring in a
previous comment..

>
>
> > +int spc_mode_sense(int host_no, struct scsi_cmd *cmd, struct list_head * mode_head)
> > +{
> > +     int             ret = SAM_STAT_GOOD, len;
>
> Should be:
>
> int ret = SAM_STAT_GOOD, len;

OK, I've a habit of inserting a '<tab>' char here. I'll correct my ways. :)

>
>
> > +     uint8_t         pcode = cmd->scb[2] & 0x3f;
> > +     uint64_t        size;
> > +     uint8_t         *data = NULL;
> > +     unsigned char   key = ILLEGAL_REQUEST;
> > +     uint16_t        asc_ascq = E_INVALID_FIELD_IN_CDB;
> > +     struct  mode    * m;
>
> Can you read the Linux coding style document, fix and resubmit the
> patches? If this is the last patchset that you send (hopefully not),
> I'll fix them this time.

Hopefully it is just the beginning.

My aim is for complete SMC / SSC / SPC + other SCSI-3 support.

>
> Please try `git-apply --whitespace=error` before submitting a patch to
> avoid trailing spaces.
>

Cheers
Mark