Hi Matthias,
On Fri, Oct 24, 2008 at 06:56:26PM +0200, Matthias Koenig wrote:
> As swap format depends on the pagesize being used, it may happen
> that the pagesize of the swap space and the current pagesize differ,
when? Kernel update, move a disk between machines?
> resulting in swapon to fail when trying to enable such a swap space.
> In such a case swapon should rather reinitialize the swap space.
OK.
> +#include <byteswap.h>
see include/bitops.h
> #include "xmalloc.h"
> #include "swap_constants.h"
> #include "nls.h"
> @@ -39,6 +41,33 @@
> #define QUIET 1
> #define CANONIC 1
>
> +#define MAX_PAGESIZE (64 * 1024)
> +static unsigned int page_size[] = { 4 * 1024,
> + 8 * 1024,
> + 16 * 1024,
> + 64 * 1024,
> + 0 };
you needn't this array
> +struct swap_info {
> + char bootbits[1024]; /* Space for disklabel etc. */
> + uint32_t version;
> + uint32_t last_page;
> + uint32_t nr_badpages;
> + unsigned char sws_uuid[16];
> + unsigned char sws_volume[16];
> + uint32_t padding[117];
> + uint32_t badpages[1];
> +};
see disk-utils/swapheader.h
Maybe we need to move this header file to the include/ directory.
> +enum {
> + SWAP_V0 = 1,
> + SWAP_V1,
> + SWAP_SUSPEND,
> + SWAP_ULSUSPEND,
> + SWAP_OOTSUSPEND
> +};
It seems you needn't this enum, your code checks for true/false of
swap_detect_signature().
> +int
> +swap_detect_signature(const char *buf)
> +{
> + if (memcmp(buf, "SWAP-SPACE", 10) == 0)
> + return SWAP_V0;
> + else if (memcmp(buf, "SWAPSPACE2", 10) == 0)
> + return SWAP_V1;
> + else if (memcmp(buf, "S1SUSPEND", 9) == 0)
> + return SWAP_SUSPEND;
> + else if (memcmp(buf, "ULSUSPEND", 9) == 0)
> + return SWAP_ULSUSPEND;
> + else if (memcmp(buf, "\xed\xc3\x02\xe9\x98\x56\xe5\x0c", 8) == 0)
> + return SWAP_OOTSUSPEND;
You needn't to check for suspends. It's already detected by
swap_is_suspend().
> +unsigned int
> +swap_get_pagesize(const char *dev)
> +{
> + int fd;
> + char *buf;
> + unsigned int *page, last_page;
> + unsigned int pagesize = 0;
> + off_t size, swap_size;
> + int swap_version;
> + int flip = 0;
> + struct swap_info *s;
> +
> + fd = open(dev, O_RDONLY);
> + if (fd == -1) {
> + perror("open");
> + return 0;
> + }
> +
> + /* get size */
> + size = lseek(fd, 0, SEEK_END);
> + if (size == (off_t)-1) {
> + perror("lseek");
> + goto err;
> + }
> + /* rewind */
> + if (lseek(fd, 0, SEEK_SET)) {
> + perror("lseek");
> + goto err;
> + }
The lseek() is overkill for standard block devices. See
disk-utils/mkswap.c and lib/blkdev.c.
> + buf = malloc(MAX_PAGESIZE);
> + if (!buf) {
> + perror("malloc");
> + goto err;
> + }
> +
> + if (read(fd, buf, MAX_PAGESIZE) == (ssize_t)-1) {
> + perror("read");
> + goto err1;
> + }
> +
> + for (page = page_size; *page; ++page) {
for (page = 0x1000; page <= MAX_PAGESIZE; page <<= 1) {
it's simpler that the page_size array
> + char *off = buf + *page - 10;
> + if (swap_detect_signature(off)) {
> + pagesize = *page;
> + break;
> + }
> + }
> + if (pagesize) {
> + s = (struct swap_info *)buf;
> + if (s->version == 1)
> + last_page = s->last_page;
> + else if (bswap_32(s->version) == 1) {
> + flip = 1;
> + last_page = bswap_32(s->last_page);
> + }
{le,be}32_to_cpu() from bitops.h
> + if (verbose)
> + fprintf(stderr, _("found %sswap v%d signature string"
> + " for %d KiB PAGE_SIZE\n"),
> + flip ? "other-endian " : "", swap_version - 1,
> + pagesize / 1024);
where do you initialize the "swap_version" variable?
> + swap_size = (last_page + 1) * pagesize;
> + if (swap_size > size) {
> + if (verbose)
> + fprintf(stderr, _("last_page 0x%08llx is larger"
> + " than actual size of swapspace\n"),
> + (unsigned long long)swap_size);
> + pagesize = 0;
> + }
> + }
> +
> +err1:
> + free(buf);
> +err:
> + close(fd);
> + return pagesize;
> +}
> +
> static int
> do_swapon(const char *orig_special, int prio, int canonic) {
> int status;
> + int reinitialize = 0;
> struct stat st;
> const char *special = orig_special;
> + unsigned int pagesize = sysconf(_SC_PAGESIZE);
Please use getpagesize(). It should be reliable (now) and more portable
to non-glibc environments.
> + unsigned int swap_pagesize = 0;
>
> if (verbose)
> printf(_("%s on %s\n"), progname, orig_special);
> @@ -260,6 +392,15 @@ do_swapon(const char *orig_special, int prio, int canonic) {
> return -1;
> }
>
> + swap_pagesize = swap_get_pagesize(special);
> + if (swap_pagesize && (pagesize != swap_pagesize)) {
> + if (verbose)
> + fprintf(stderr, _("swap format pagesize does not match."
> + " Reinitializing the swap.\n"),
> + progname, special);
> + reinitialize = 1;
> + }
.. one question comes to mind: is it safe? Are you really sure that
there is a swap space on the device? Maybe I'm too paranoid, but I
I think it would be better to ask libblkid/volume_id.
main() {
...
type = fsprobe_get_fstype_by_devname(device);
if (strcmp(type, "swap") == 0) {
if (!swap_pagesize_is_valid(device))
reinitialize = 1;
} else if (strcmp(type, "suspend") == 0 ||
strcmp(type, "swsuspend") == 0)
reinitialize = 1;
...
}
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe util-linux-ng" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
