lkml.org 
[lkml]   [2007]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] Add iSCSI IBFT Support (v0.3)
    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)
    > +{
    > + struct ibft_initiator *initiator = attr->initiator;
    > + void *ibft_loc = entry->data->hdr;
    > + char *str = buf;
    > +
    > + if (!initiator)
    > + return 0;
    > +
    > + str += sprintf_ipaddr(str, "isns", initiator->isns_server);
    > + str += sprintf_ipaddr(str, "slp", initiator->slp_server);
    > + str += sprintf_ipaddr(str, "primary_radius_server",
    > + initiator->pri_radius_server);
    > + str += sprintf_ipaddr(str, "secondary_radius_server",
    > + initiator->sec_radius_server);
    > + str += sprintf_string(str, "itname", initiator->initiator_name_len,
    > + (char *)ibft_loc + initiator->initiator_name_off);
    > + str--;
    > +
    > + return str-buf;
    > +}

    sysfs files have ONE VALUE PER FILE, not a whole bunch of different
    things in a single file. Please fix this.


    > +
    > +ssize_t ibft_attr_show_nic(struct ibft_kobject *entry,
    > + struct ibft_attribute *attr,
    > + char *buf)
    > +{
    > + struct ibft_nic *nic = attr->nic;
    > + void *ibft_loc = entry->data->hdr;
    > + char *str = buf;
    > +
    > + if (!nic)
    > + return 0;
    > + /*
    > + * Assume dhcp if any non-zero portions of its address are set.
    > + */
    > + if (memcmp(nic->dhcp, nulls, sizeof(nic->dhcp))) {
    > + str += sprintf_ipaddr(str, "dhcp", nic->dhcp);
    > + } else {
    > + str += sprintf_ipaddr(str, "ciaddr", nic->ip_addr);
    > + str += sprintf_ipaddr(str, "giaddr", nic->gateway);
    > + str += sprintf_ipaddr(str, "dnsaddr1", nic->primary_dns);
    > + str += sprintf_ipaddr(str, "dnsaddr2", nic->secondary_dns);
    > + }
    > + if (nic->hostname_len)
    > + str += sprintf_string(str, "hostname", nic->hostname_len,
    > + (char *)ibft_loc + nic->hostname_off);
    > + /* Cut off the comma. */
    > + str--;
    > +
    > + return str-buf;
    > +}

    Same here.

    > +ssize_t ibft_attr_show_target(struct ibft_kobject *entry,
    > + struct ibft_attribute *attr,
    > + char *buf)
    > +{
    > + struct ibft_tgt *tgt = attr->tgt;
    > + void *ibft_loc = entry->data->hdr;
    > + char *str = buf;
    > + int i;
    > +
    > + if (!tgt)
    > + return 0;
    > +
    > + str += sprintf_ipaddr(str, "siaddr", tgt->ip_addr);
    > + str += sprintf(str, "iport=%d,", tgt->port);
    > + str += sprintf(str, "ilun=");
    > + for (i = 0; i < 8; i++)
    > + str += sprintf(str, "%x", (u8)tgt->lun[i]);
    > + str += sprintf(str, ",");
    > +
    > + if (tgt->tgt_name_len)
    > + str += sprintf_string(str, "iname", tgt->tgt_name_len,
    > + (void *)ibft_loc + tgt->tgt_name_off);
    > +
    > + if (tgt->chap_name_len)
    > + str += sprintf_string(str, "chapid", tgt->chap_name_len,
    > + (char *)ibft_loc + tgt->chap_name_off);
    > + if (tgt->chap_secret_len)
    > + str += sprintf_string(str, "chappw", tgt->chap_secret_len,
    > + (char *)ibft_loc + tgt->chap_secret_off);
    > + if (tgt->rev_chap_name_len)
    > + str += sprintf_string(str, "ichapid", tgt->rev_chap_name_len,
    > + (char *)ibft_loc + tgt->rev_chap_name_off);
    > + if (tgt->rev_chap_secret_len)
    > + str += sprintf_string(str, "ichappw", tgt->rev_chap_secret_len,
    > + (char *)ibft_loc + tgt->rev_chap_secret_off);
    > +
    > + /* Cut off the comma. */
    > + str--;
    > +
    > + return str-buf;
    > +}

    Same here, are we writing a novella or something to userspace? :)

    > +ssize_t ibft_attr_show_disk(struct ibft_kobject *dev,
    > + struct ibft_attribute *ibft_attr,
    > + char *buf)
    > +{
    > + char *str = buf;
    > +
    > + str += sprintf(str, "//ethernet@0,%d:iscsi,", dev->data->index);
    > + str += ibft_attr_show_initiator(dev, ibft_attr, str);
    > + str += sprintf(str, ",");
    > + str += ibft_attr_show_target(dev, ibft_attr, str);
    > + str += sprintf(str, ",");
    > + str += ibft_attr_show_nic(dev, ibft_attr, str);
    > +
    > + return str-buf;
    > +}

    And here, do I need to go on?

    > +ssize_t ibft_attr_show_mac(struct ibft_kobject *entry,
    > + struct ibft_attribute *attr,
    > + char *buf)
    > +{
    > + struct ibft_nic *nic = attr->nic;
    > + int len = 6;
    > +
    > + if (!nic)
    > + return 0;
    > +
    > + 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...

    > +/*
    > + * 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)
    > +{
    > + struct ibft_kobject *dev =
    > + container_of(kobj, struct ibft_kobject, kobj);
    > + struct ibft_attribute *ibft_attr =
    > + container_of(attr, struct ibft_attribute, attr);
    > + ssize_t ret = -EIO;
    > + char *str = buf;
    > +
    > + if (!capable(CAP_SYS_ADMIN))
    > + return -EACCES;
    > +
    > + if (ibft_attr->show)
    > + ret = ibft_attr->show(dev, ibft_attr, str);
    > +
    > + return ret;
    > +}
    > +
    > +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.

    > +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 :(

    > +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
    > +
    > +#define IBFT_SIGN "iBFT"
    > +#define IBFT_SIGN_LEN 4
    > +#define IBFT_START 0x80000 /* 512kB */
    > +#define IBFT_END 0x100000 /* 1MB */
    > +#define VGA_MEM 0xA0000 /* VGA buffer */
    > +#define VGA_SIZE 0x20000 /* 132kB */
    > +static ssize_t find_ibft(void)
    > +{
    > + unsigned long pos;
    > +
    > + for (pos = IBFT_START; pos < IBFT_END; pos += 16) {
    > + void *virt;
    > + /* The table can't be inside the VGA BIOS reserved space,
    > + * so skip that area */
    > + if (pos == VGA_MEM)
    > + pos += VGA_SIZE;
    > + virt = phys_to_virt(pos);
    > + if (memcmp(virt, IBFT_SIGN, IBFT_SIGN_LEN) == 0) {
    > + unsigned long *addr =
    > + (unsigned long *)phys_to_virt(pos + 4);
    > + unsigned int len = *addr;
    > + /* if the length of the table extends past 1M,
    > + * the table cannot be valid. */
    > + if (pos + len <= (IBFT_END-1)) {
    > + ibft_phys = (void *)pos;
    > + return len;
    > + }
    > + }
    > + }
    > + return 0;
    > +}

    What is a function (not even an inline one) doing in a .h file?

    > +enum ibft_kobject_type {
    > + kobject_type_chosen,
    > + kobject_type_ethernet,
    > + kobject_type_target,
    > + kobject_type_aliases
    > +};
    > +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?

    > + struct list_head node;
    > +};
    > +
    > +/*
    > + * The text attributes, which can be:
    > + * - local-mac-address (many of them)
    > + * - property (many of them)
    > + * - bootpath (one) , iscsi-diskX (many)
    > + *
    > +*/
    > +#define IBFT_ISCSI_ATTR_DISK_NAME "iscsi-disk%d"
    > +#define IBFT_ISCSI_ATTR_PROPERTY_NAME "property"
    > +#define IBFT_ISCSI_ATTR_BOOTPATH_NAME "bootpath"
    > +#define IBFT_ISCSI_ATTR_LOCAL_MAC_ADDRESS_NAME "local-mac-address"
    > +#define IBFT_ISCSI_ATTR_MAX_LEN 18
    > +
    > +struct ibft_attribute {
    > + struct attribute attr;
    > + ssize_t (*show) (struct ibft_kobject *entry,
    > + struct ibft_attribute *attr, char *buf);
    > + struct ibft_initiator *initiator;
    > + struct ibft_nic *nic;
    > + struct ibft_tgt *tgt;
    > + struct kobject *kobj;
    > + 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.

    > + struct list_head node;
    > +};


    thanks,

    greg k-h
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-11-27 04:35    [W:0.037 / U:1.516 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site