Re: [Qemu-devel] [PATCH] block/raw-posix: get right size of partition size

[Christoph Egger <Christoph.Egger@amd.com>]

On 05/23/11 13:06, Christoph Hellwig wrote:
> On Mon, May 23, 2011 at 12:34:39PM +0200, Christoph Egger wrote:
>>
>> This does 2 things:
>> - use the correct way to get the size of a disk device or partition
>>      (from haad@NetBSD.org)
>> - if given a block device, use the character device instead.
>>      (from bouyer@NetBSD.org)
>
> Please split that into two independent patches.

ok.

>
>> +    if (S_ISLNK(sb.st_mode)) {
>> +        fprintf(stderr, "%s: symbolic link not supported\n", filename);
>> +        return -EINVAL;
>> +    }
>
> Why not, it's a pretty clear regression from current code, and will
> break various Linux setups where there are lots of symlinks under /dev,
> as well as users using symlinks for their image files.

there's no easy way to find the real target of a symlink, to find the 
corresponding character device (this could be refinded, because this 
restriction only applies if the target is a block device).

>
>> +#if defined(__NetBSD__)
>> +    if (S_ISBLK(sb.st_mode)) {
>> +        static char namebuf[PATH_MAX];
>> +        const char *dp = strrchr(filename, '/');
>> +
>> +        if (dp == NULL) {
>> +            snprintf(namebuf, PATH_MAX, "r%s", filename);
>> +        } else {
>> +            snprintf(namebuf, PATH_MAX, "%.*s/r%s",
>> +                (int)(dp - filename), filename, dp + 1);
>> +        }
>> +        fprintf(stderr, "%s is a block device", filename);
>> +        filename = namebuf;
>> +        fprintf(stderr, ", using %s\n", filename);
>> +    }
>> +#endif
>
> Please split this out into a helper, which has a no-op version
> for other operating systems.  It probably should also be enabled
> for other operating systems having char and block nodes for disk
> devices.  That's at least OpenBSD and Solaris, not sure about
> Darwin.

Yes, but don't have a test machine right now.

>> -#ifdef __OpenBSD__
>> +#if defined(__OpenBSD__) || defined(__NetBSD__)
>>   static int64_t raw_getlength(BlockDriverState *bs)
>>   {
>>       BDRVRawState *s = bs->opaque;
>> @@ -613,12 +645,22 @@ static int64_t raw_getlength(BlockDriverState *bs)
>>       if (fstat(fd,&st))
>>           return -1;
>>       if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
>> -        struct disklabel dl;
>> +#if defined(__NetBSD__)
>> +        struct dkwedge_info dkw;
>>
>> -        if (ioctl(fd, DIOCGDINFO,&dl))
>> -            return -1;
>> -        return (uint64_t)dl.d_secsize *
>> -            dl.d_partitions[DISKPART(st.st_rdev)].p_size;
>> +        if (ioctl(fd, DIOCGWEDGEINFO,&dkw) != -1) {
>> +            return dkw.dkw_size * 512;
>> +        } else {
>> +#endif
>
> Please provide a completely separate raw_getlength for NetBSD instead
> of creating this ifdef mess for almost no shared code.

Ok.


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632