On Monday 26 November 2007 22:31:38 Greg KH wrote:
> On Mon, Nov 26, 2007 at 06:56:42PM -0400, Konrad Rzeszutek wrote:
> > +/*
> > + * Routines for reading of the iBFT data in a human readable fashion.
> > + */
> > +ssize_t ibft_attr_show_initiator(struct ibft_kobject *entry,
> > + struct ibft_attribute *attr,
> > + char *buf)
> > +{
.. snip..
> > +
> > + str += sprintf_ipaddr(str, "isns", initiator->isns_server);
> > + str += sprintf_ipaddr(str, "slp", initiator->slp_server);
.. snip ..
>
> sysfs files have ONE VALUE PER FILE, not a whole bunch of different
> things in a single file. Please fix this.
No problem. I will have that shortly posted.
>
> > +
> > +ssize_t ibft_attr_show_nic(struct ibft_kobject *entry,
> > + struct ibft_attribute *attr,
> > + char *buf)
.. snip..
> > + str += sprintf_ipaddr(str, "giaddr", nic->gateway);
> > + str += sprintf_ipaddr(str, "dnsaddr1", nic->primary_dns);
>
> Same here.
Yup.
>
> > +ssize_t ibft_attr_show_target(struct ibft_kobject *entry,
> > + struct ibft_attribute *attr,
> > + char *buf)
> > +{
.. snip..
> > +}
>
> Same here, are we writing a novella or something to userspace? :)
Hehe.. I will make it simpler :-)
>
> > +ssize_t ibft_attr_show_disk(struct ibft_kobject *dev,
> > + struct ibft_attribute *ibft_attr,
> > + char *buf)
> > +{
.. snip ..
> > +}
>
> And here, do I need to go on?
I will have a new version posted quite shortly.
>
> > +ssize_t ibft_attr_show_mac(struct ibft_kobject *entry,
> > + struct ibft_attribute *attr,
> > + char *buf)
> > +{
..snip..
> > +
> > + memcpy(buf, attr->nic->mac, len);
> > +
> > + return len;
> > +}
>
> Is mac a user readable string? Then perhaps a simple sprintf would work
> instead, as I doubt you are including a \n here...
It was meant to be as a binary value. But that doesn't fit in sysfs directory,
so let me make it use sprintf here.
>
> > +/*
> > + * The main routine which allows the user to read the IBFT data.
> > + */
> > +static ssize_t ibft_show_attribute(struct kobject *kobj,
> > + struct attribute *attr,
> > + char *buf)
> > +{
..snip..
> > +
> > +static struct sysfs_ops ibft_attr_ops = {
> > + .show = ibft_show_attribute,
> > +};
>
> I think this whole mess can go away in the new rework Kay and I have
> done, please document this whole thing and I'll see what I can do.
Absolutely.
>
> > +struct ibft_control {
> > + struct ibft_hdr hdr;
> > + u16 extensions;
> > + u16 initiator_off;
> > + u16 nic0_off;
> > + u16 tgt0_off;
> > + u16 nic1_off;
> > + u16 tgt1_off;
> > +} __attribute__((__packed__));
>
> Did we loose tabs for some reason? I'm guessing your editor is not
> showing them properly, nor did you use scripts/checkpatch.pl :(
I did use checkpatch.pl v0.99 downloaded somewhere from the web. I hadn't
realized it was now residing in scripts/checkpatch.pl - and from now on I will
use that.
>
> > +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
..snip..
> > +static ssize_t find_ibft(void)
> > +{
..snip..
> > +}
>
> What is a function (not even an inline one) doing in a .h file?
I was not sure where to put it. This function (find_ibft) is used by the
setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in .c file,
but I am not sure exactly where? Should I make a new file in called
libs/iscsi_ibft_helper.c ?
>
..snip..
> > +struct ibft_kobject {
> > + struct ibft_data *data;
> > + char name[IBFT_ISCSI_KOBJECT_MAX_LEN];
>
> Why have this,
>
> > + u8 type;
> > + struct kobject kobj;
>
> When the kobject itself has an unlimited size name associated with it?
Absolutely no reason at all. It was a evolution vestige of the code that is
not needed anymore.
>
..snip..
> > + char name[IBFT_ISCSI_ATTR_MAX_LEN];
>
> Same here, an attribute already has a pointer to a name, no need to have
> another one in the same structure.
Thanks. Will remove it.
>
> > + struct list_head node;
> > +};
>
> thanks,
Thank you for taking your time to review the code. I will have the new
version out shortly.
>
> greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
