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

[Christoph Egger]

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 address@hidden)
> > - if given a block device, use the character device instead.
> >      (from address@hidden)
>
> 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