Messages in this thread | | | Date | Thu, 28 Sep 2006 15:42:37 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH -mm 1/3] swsusp: Add ioctl for swap files support |
| |
On Fri, 29 Sep 2006 00:13:38 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> To be able to use swap files as suspend storage from the userland suspend > tools we need an additional ioctl() that will allow us to provide the kernel > with both the swap header's offset and the identification of the resume > partition. > > The new ioctl() should be regarded as a replacement for the > SNAPSHOT_SET_SWAP_FILE ioctl() that from now on will be considered as > obsolete, but has to stay for backwards compatibility of the interface. > > + > +/* > + * This structure is used to pass the values needed for the identification > + * of the resume swap area from a user space to the kernel via the > + * SNAPSHOT_SET_SWAP_AREA ioctl > + */ > +struct resume_swap_area { > + u_int16_t dev; > + loff_t offset; > +} __attribute__((packed)); >
hmm. Asking the compiler to pack 16-bit and 64-bit quantities in this manner is a bit risky. I guess it'll do the right thing, consistently, across all compiler versions and vendors and 32-bit-on-64-bit-kernel, etc.
But from a defensiveness/paranoia POV it'd be better to use a u32 here, I suspect. (Will access to that loff_t cause an alignment trap on ia64? Any other CPUs? Dunno).
> #define PMOPS_PREPARE 1 > #define PMOPS_ENTER 2 > Index: linux-2.6.18-mm1/kernel/power/user.c > =================================================================== > --- linux-2.6.18-mm1.orig/kernel/power/user.c > +++ linux-2.6.18-mm1/kernel/power/user.c > @@ -343,6 +343,37 @@ OutS3: > } > break; > > + case SNAPSHOT_SET_SWAP_AREA: > + if (data->bitmap) { > + error = -EPERM; > + } else { > + struct resume_swap_area swap_area; > + dev_t swdev; > + > + error = copy_from_user(&swap_area, (void __user *)arg, > + sizeof(struct resume_swap_area)); > + if (error) { > + error = -EFAULT; > + break; > + } > + > + /* > + * User space encodes device types as two-byte values, > + * so we need to recode them > + */
Really? stat() uses unsigned long and stat64() uses unsigned long long dev_t.
> + swdev = old_decode_dev(swap_area.dev); > + if (swdev) { > + offset = swap_area.offset; > + data->swap = swap_type_of(swdev, offset); > + if (data->swap < 0) > + error = -ENODEV; > + } else { > + data->swap = -1; > + error = -EINVAL; > + } > + } > + break; > + > default: > error = -ENOTTY;
But I wonder if we need to pass the device identified into this ioctl at all. What device is the ioctl() against? ie: what do `filp' and `inode' point at? If it's /dev/hda1 then everything we need is right there, is it not?
ohshit, it's a miscdevice. I wonder if it would have defined all this stuff to be operations against the blockdev. Perhaps not.
Well anyway. It might be neater to require that userspace open /dev/hda1 and pass in the fd to this ioctl. But this code will never be neat, so whatever.
- 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/
| |