Functions and Structures

[mark3094]

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?

[Combuster]

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?

[gerryg400]

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.

[Solar]

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

[mark3094]

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

[mark3094]

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.

[gerryg400]

Yes, you need to malloc.

[Combuster]

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.

[mark3094]

Yes, you need to malloc.

Thankyou, I will look into writing a malloc function

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

[Solar]

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;

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.

[mark3094]

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.

[mark3094]

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.