OSDev.org

Posted: **Tue Sep 20, 2011 12:30 am**

I've been developing a function which uses PIO to identify hard drive features (model, number of sectors, etc), and stores it in a structure, such as pri_master.model, pri_master.sectors, etc:

Code: Select all

struct ata_info {
	uword config;
	char serial[21];
	char firmware[9];
	char model[41];
	uword cylinders;
	uword heads;
	uword sectors;
	udword totalsectors;
	udword size;
};

Currently I have a function (returning void) that sets these values for the master disk on the primary channel. I would also like to get information on the secondary channel and on slave disks. I could do this by writing three more additional functions that set pri_slave.model, etc, but this would be a big waste.

I have tried creating a function to return a struct, similar to what is shown here:
http://www.java2s.com/Tutorial/Cpp/0160 ... nction.htm (this is in C++, but I am using C)

The 'config' member returns OK, but the rest returns garbage. Also, I have read that doing this is slow, and can cause a stack overflow.

Is there a better way to do this?

Posted: **Tue Sep 20, 2011 12:39 am**

Is there a better way to do this?

The typical solution would take the form of

Code: Select all

int pata_check_drive(uint16_t baseport, int master_or_slave, ata_info * pointer_to_storage)

In the end, saving on code duplication is always better than premature optimisation. Can you see what all inputs and outputs are for?

Posted: **Tue Sep 20, 2011 12:52 am**

or

Code: Select all

struct ata_info *ata_create(uint16_t baseport, int master_or_slave) {

    uint16_t           buf[256];
    struct ata_info  *pinfo;

    pinfo = (struct ata_info *)malloc(sizeof(struct ata_info *));
    if (!pinfo) {
        return NULL;
    }

    // Code to read all the data from the PIO port
    status = read_info_to_buffer_and_check_status(buf);
    if (status == -1) {
        free(pinfo);
        return NULL;
    }

    pinfo->config = buf[0];
    pinfo->cylinders = buf[1];
    etc.

    return pinfo;
}

You'll need an ata_destroy() as well.

Posted: **Tue Sep 20, 2011 1:31 am**

Tip #1: Use uint16_t, uint32_t and their <stdint.h> kind, instead of uword / udword etc.; saves you one custom-made header and makes your code more expressive. (This is a matter of personal taste, though.)

Tip #2: Functions returning structs are discouraged. You get into all kind of trouble because it is not clear to the casual observer who is responsible for allocating / deallocating the memory used by the structure. (I.e., they are a recipee for unreleased resources / memory leaks.) Either the function administrates the structures internally (and returns a pointer to them, like fopen() / fclose()), or the function is given an appropriate pointer to a user-allocated struct as a parameter (like in Combuster's example).

Posted: **Tue Sep 20, 2011 4:10 pm**

Thankyou all for your help.

I found the same answer (that is, using pointers) myself this morning, before finding your reply posts. All of you have posted some other really good tips, so I will be looking into those as well (everyone loves a bonus).

Thanks again for your help

Posted: **Thu Sep 22, 2011 6:06 am**

I have a small problem.
My implementation is fairly similar to the one recommended by gerry400.

In my header I declare a struct, ata_info, and declare my pointers, one for each possbile ATA hard drive:

Code: Select all

static struct ata_info *pri_master;
static struct ata_info *pri_slave;
static struct ata_info *sec_master;
static struct ata_info *sec_slave;

I set the members in these pointer structures by calling the identify function:

Code: Select all

pri_master = identify(PRI_CONTROLLER, MASTER_DISK);
pri_slave = identify(PRI_CONTROLLER, SLAVE_DISK);

They both detect the ATA information correctly, but calling the pri_slave = ... function seems to overwrite the pri_master pointer. So, if I go to print any of the members of pri_master, the results in pri_slave are printed.

A cut down version of the identify function is:

Code: Select all

struct ata_info *identify(uword controller, ubyte drive) {
	uword buffer[256];
	struct ata_info *funcptr = {0};
	...
	funcptr->atapi = buffer[0];
	return funcptr;

Do I need to allocate memory with malloc()? (I don't have a malloc function yet, but I can write one if I have to)
Or do I need to have a function to destroy the funcptr pointer, as gerryg suggested? If so, how would I go about that?

All help is appreciated.

Posted: **Thu Sep 22, 2011 6:29 am**

Yes, you need to malloc.

Posted: **Thu Sep 22, 2011 6:58 am**

Code: Select all

   struct ata_info *funcptr = {0};
   funcptr->atapi = buffer[0];

First you need to understand why that example code of yours crashes before continuting OS development in this language.

Posted: **Thu Sep 22, 2011 3:38 pm**

gerryg400 wrote:Yes, you need to malloc.

Thankyou, I will look into writing a malloc function

Combuster wrote:First you need to understand why that example code of yours crashes before continuting OS development in this language.

It's not actually crashing, just displaying the wrong information. If you're referring to the atapi member of the structure, it's not listed in my first post as I have added it since then. It is used instead of the config member to determine if a device is ATA or ATAPI.

If you're referring to the *funcptr = {0} part, that was an attempt to initialise the pointer (as recommended by an article I found), as the compiler was complaining about it.
buffer contains the words that have been read from the disk during identify. The code has been omitted.

Thanks again

Posted: **Fri Sep 23, 2011 6:35 am**

mark3094 wrote:
Code: Select all
struct ata_info *identify(uword controller, ubyte drive) {
	uword buffer[256];
	struct ata_info *funcptr = {0};
	...
	funcptr->atapi = buffer[0];
	return funcptr;
Combuster wrote:First you need to understand why that example code of yours crashes before continuting OS development in this language.
It's not actually crashing, just displaying the wrong information.

Whatever funcptr pointed to while the function was running, it isn't there anymore because your stackframe just got released.

Welcome to "undefined behaviour" country.

If that statement doesn't immediately hit a button in your head ("how could I have been that stupid?"), or worse, doesn't even make immediate sense to you, you shouldn't be doing kernel-space work. Please gather experience writing user-space applications. Kernel space is a hostile environment where errors like this one must not happen anymore.

Posted: **Sat Sep 24, 2011 5:39 am**

solar wrote:If that statement doesn't immediately hit a button in your head ("how could I have been that stupid?"), or worse, doesn't even make immediate sense to you, you shouldn't be doing kernel-space work. Please gather experience writing user-space applications. Kernel space is a hostile environment where errors like this one must not happen anymore.

Yes indeed. One of those moments.

Posted: **Sat Oct 01, 2011 6:19 pm**

Fixed. Sorry for any inconvenience.

I just selected the text and clicked 'quote', so I'm not sure what went wrong. I will double check next time I quote something.

OSDev.org

Functions and Structures

Functions and Structures

Re: Functions and Structures

Re: Functions and Structures

Re: Functions and Structures

Re: Functions and Structures

Re: Functions and Structures

Re: Functions and Structures

Re: Functions and Structures

Re: Functions and Structures

Re: Functions and Structures

Re: Functions and Structures

Re: Functions and Structures