[libvirt] [PATCH]: Export the 'label' on block devices

[Chris Lalancette]

Jim Meyering wrote:
>> diff -u -r1.21 storage_backend.c
>> --- src/storage_backend.c	5 Sep 2008 12:03:45 -0000	1.21
>> +++ src/storage_backend.c	16 Oct 2008 07:29:46 -0000
>> @@ -192,6 +192,30 @@
>>      return ret;
>>  }
>>
>> +struct diskType disk_types[] = {
>> +    { "lvm2", VIR_STORAGE_POOL_DISK_LVM2, 0x218, 8, 0x31303020324D564CUL },
>> +    { "gpt",  VIR_STORAGE_POOL_DISK_GPT,  0x200, 8, 0x5452415020494645UL },
>> +    { "dvh",  VIR_STORAGE_POOL_DISK_DVH,  0x0,   4, 0x41A9E50BUL },
>> +    { "mac",  VIR_STORAGE_POOL_DISK_MAC,  0x0,   2, 0x5245UL },
>> +    { "bsd",  VIR_STORAGE_POOL_DISK_BSD,  0x40,  4, 0x82564557UL },
>> +    { "sun",  VIR_STORAGE_POOL_DISK_SUN,  0x1fc, 2, 0xBEDAUL },
> 
> Some partition tables are deliberately created to be dual GPT/DOS-based.
> Here, it gets the first match (as you've deliberately ordered them that
> way, I see).  We might need a mechanism to let the caller prefer one or
> the other.  Also, realize that this can be wrong if there was once a GPT
> table, and it has since been replaced with a DOS one that still happens
> to have the GPT signature bytes.  Or maybe it was dual GPT/DOS long ago
> but has since been maintained only as DOS (in which case all GPT-related
> headers could be gone).  This suggests that pronouncing a partition
> table to have type GPT based solely on the first 1KB is a little risky.
> But it's probably just fine for now.

Sigh, yes.  As you can probably tell, this code is heavily based off of the
implementation in parted.  I really just looked at the "probe" functions for
each of the label types (more on the label name below), and followed what parted
did.  In the GPT case, it seems like it just probes the front, but you've
pointed out via IRC that other routines do checking (like in the last sector,
for the backup signature).  Like you say, though, I *think* this will be OK for
now; if we really need it, we can add "multiple signature checking" later.

> 
>> +    /*
>> +     * NOTE: pc98 is funky; the actual signature is 0x55AA (just like dos), so
>> +     * we can't use that.  At the moment I'm relying on the "dummy" IPL
>> +     * bootloader data that comes from parted.  Luckily, the chances of running
>> +     * into a pc98 machine running libvirt are approximately nil.
> 
> I agree.
> Why not just exclude it?

I'd be happy to.  I mostly included it because it was already there in the
storage_backend_disk implementation, and it was relatively easy to add.  If no
one else has objections, I'm happy to dump it over the side.

>> +    if (S_ISBLK(sb.st_mode)) {
>> +        off_t start;
>> +        int i;
>> +        unsigned char buffer[1024];
>> +        ssize_t bytes;
>> +
>> +        start = lseek(fd, 0, SEEK_SET);
>> +        if (start < 0) {
>> +            virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
>> +                                  _("cannot seek to beginning of file '%s':%s"),
>> +                                  vol->target.path, strerror(errno));
>> +            return -1;
>> +        }
>> +        memset(buffer, 0, 1024);
> 
> You can remove the above memset, given one of the suggestions below.
> 
>> +        bytes = saferead(fd, buffer, 1024);
> 
> Better not to repeat constants like that.  use sizeof:
> 
>            bytes = saferead(fd, buffer, sizeof buffer);

Yeah, good point.  Will fix.

> 
>> +        if (bytes < 0) {
>> +            virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
>> +                                  _("cannot read beginning of file '%s':%s"),
>> +                                  vol->target.path, strerror(errno));
>> +            return -1;
>> +        }
> 
> At worst, memset just the probably-empty portion of the buffer
> that wasn't set by the read:
> 
>            memset(buffer + bytes, 0, sizeof buffer - bytes);
> 
> However, I prefer to drop the memset altogether and check
> offset+len against "bytes" below:
> 
>> +        for (i = 0; disk_types[i].name != NULL; i++) {
> 
> It'd be good to check that offset+len is within range, regardless
> of whether the buffer is zeroed.  In case someone decides to shrink
> buffer or to add a type with magic bytes past the 1KB mark.
> 
>                if (disk_types[i].offset + disk_types[i].length > bytes)
>                    continue;

Ah, yes, I see.  Yeah, I thought about adding additional checking for this, but
just didn't do it.  Your suggestion is good, I'll do that.

>> diff -u -r1.7 storage_backend.h
>> --- src/storage_backend.h	2 Sep 2008 14:15:42 -0000	1.7
>> +++ src/storage_backend.h	16 Oct 2008 07:29:46 -0000
>> @@ -56,6 +56,30 @@
>>      VIR_STORAGE_BACKEND_POOL_SOURCE_NAME    = (1<<4),
>>  };
>>
>> +enum diskLabel {
>> +    VIR_STORAGE_POOL_DISK_DOS = 0,
> 
> I like to start with 1 or larger, so that using an uninitialized
> variable (often 0) is more likely to trigger an error.

In general, I agree with you, and if I was writing this part from scratch I
would do that.  I left it as-is because this is the way it was already done.
Actually this goes along with the whole "slightly changing semantics" thing I
mentioned for storage_backend_disk.  Because the vol pointer is always allocated
with VIR_ALLOC (i.e. calloc), it means that this field is always 0.  That's why
"unknown" partition table types always defaulted to DOS, since DOS was the
"0'th" enum entry.  If we are going to change the semantic, though, we can bump
this up to start at 1, and then the 0 entry will always return "unknown", like I
think it should.

>> +struct diskType {
>> +    const char *name;
>> +    /* the 'label' terminology comes from parted */
> 
> i find "label" (meaning "partition table") terminology confusing, if only
> because each partition can have a volume-label string.  If it weren't so
> ensconced in Parted's code, directory structure (libparted/labels/*.c)
> and documentation, I would have excised it long ago.  I much prefer
> "partition table".
> 
> In a way, I don't mean to make you change names, but I'd hate
> to see Parted's poorly chosen name propagate any more than it has.

Understood.  I had a hard time coming up with a name, because "partition table"
tends to get people thinking about FAT16, Linux, etc (which is the wrong thing;
those are partition types, not partition table types, but this whole area is
confusing).  I thought the parted terminology was somehow standard, so I took
that.  I guess I'll change it to "enum partTableType part_table", which is more
technically correct.

-- 
Chris Lalancette