Karel Zak <kzak@xxxxxxxxxx> writes:
> 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?
Update, e.g. ppc to ppc64.
>> 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
Ok
>
>> #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
Originally I did it the way it is in libvolume_id, but
then we discussed if 32k pages are really supported anywhere
and decided to skip the 32k pagesize. I found the array of
supported pagesizes approach cleaner than skipping it manually
in the loop.
>
>> +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
Ok.
Uhm... how did I miss that?
>
> 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().
Yes, we don't need this.
This was just an you-never-know-if-you-will-need-it-some-day approach.
>
>> +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().
Ok.
>
>> +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.
Ok.
>
>> + 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
See above.
>
>> + 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
This was from Olaf, I think the intention was here (Olaf please correct
I am wrong), that we don't know the endianess of the swap. If we have
now a swap with a different endianess (howsoever this has happened)
we cannot just use {le,be}32_to_cpu here.
But of course we should use the macros 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?
Hmm, somehow it got lost :(
>
>> + 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.
Ok.
>
>> + 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.
Yes, being paranoid was the reason for Olaf to insist on the size check.
Only if 1. the magic is found *and* 2. the size is valid, then we accept
this as valid swap space. I think this is more safe then current
detection in volume_id or libblkid, which mostly check for the magic
only. But of course I agree that we should rather enhance the detection in
libvolume_id/libblkid and not inherit yet another swap detetion code at
this place.
Thanks,
Matthias
--
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
