Re: [Qemu-devel] [PATCH] block/raw-posix: use a character device if a block device is given

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

On 05/23/11 16:11, Kevin Wolf wrote:
> Am 23.05.2011 14:34, schrieb Christoph Egger:
>>
>> if given a block device, use the character device instead.
>>
>> From: Manuel Bouyer<bouyer@NetBSD.org>
>> Signed-off-by: Christoph Egger<Christoph.Egger@amd.com>
>
> A useful commit message would explain why you're doing that.

How about this:

On NetBSD, the PV backend has to use the block device; but a
userland process is better with the character device interface. In
addition, a block device can't be opened twice; if the backend opens
it qemu can't and vice-versa.

>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 6b72470..d05f373 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -136,11 +143,45 @@ static int64_t raw_getlength(BlockDriverState *bs);
>>    static int cdrom_reopen(BlockDriverState *bs);
>>    #endif
>>
>> +#if defined(__NetBSD__)
>> +static const char *raw_get_rawdevice(const char *filename)
>> +{
>> +    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);
>
> Not sure if fprintf is a good idea here, but ok.

I want to make it clear what file the qemu process has been
using. this is what log files are for, isn't it ?

>> +
>> +    return filename;
>> +}
>> +#else
>> +static const char *raw_get_rawdevice(const char *filename)
>> +{
>> +    return filename;
>> +}
>> +#endif
>> +
>>    static int raw_open_common(BlockDriverState *bs, const char *filename,
>>                               int bdrv_flags, int open_flags)
>>    {
>>        BDRVRawState *s = bs->opaque;
>>        int fd, ret;
>> +    struct stat sb;
>> +
>> +    if (lstat(filename,&sb)<  0) {
>> +        fprintf(stderr, "%s: stat failed: %s\n", filename,
>> strerror(errno));
>
> The patch is corrupted by this line wrap.

this line fits into 80 columns. must be a mail client or a ms exchange
problem.

>
> Please remove the fprintf, the callers are responsible for sending an
> error message to the right destination.


>> +        return -errno;
>> +    }
>> +
>> +    if (S_ISBLK(sb.st_mode))
>> +        filename = raw_get_rawdevice(filename);
>
> The qemu coding style requires braces.
>
> Also, I agree with Christoph that the lstat/S_ISBLK should be moved into
> the NetBSD specific code.

Ok.

Christoph


-- 
---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