[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" n
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node |
Date: |
Fri, 11 Feb 2011 14:08:45 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.15) Gecko/20101027 Fedora/3.0.10-1.fc12 Thunderbird/3.0.10 |
Am 11.02.2011 13:59, schrieb Stefano Stabellini:
> On Fri, 11 Feb 2011, Kevin Wolf wrote:
>> Am 11.02.2011 13:38, schrieb Stefano Stabellini:
>>> When disk is a cdrom and the drive is empty the "params" node in
>>> xenstore might be missing completely: cope with it instead of
>>> segfaulting.
>>>
>>> Signed-off-by: Stefano Stabellini <address@hidden>
>>>
>>>
>>> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
>>> index 134ac33..e553c4c 100644
>>> --- a/hw/xen_disk.c
>>> +++ b/hw/xen_disk.c
>>> @@ -577,12 +577,13 @@ static int blk_init(struct XenDevice *xendev)
>>> {
>>> struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev,
>>> xendev);
>>> int index, qflags, have_barriers, info = 0;
>>> - char *h;
>>> + char *h = NULL;
>>>
>>> /* read xenstore entries */
>>> if (blkdev->params == NULL) {
>>> blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
>>> - h = strchr(blkdev->params, ':');
>>> + if (blkdev->params != NULL)
>>> + h = strchr(blkdev->params, ':');
>>
>> The coding style requires braces here.
>>
>
> Good point, I'll do.
>
>>> if (h != NULL) {
>>> blkdev->fileproto = blkdev->params;
>>> blkdev->filename = h+1;
>>
>> Let me add some more context:
>>
>> if (h != NULL) {
>> blkdev->fileproto = blkdev->params;
>> blkdev->filename = h+1;
>> *h = 0;
>> } else {
>> blkdev->fileproto = "<unset>";
>> blkdev->filename = blkdev->params;
>> }
>>
>> So in the NULL case we now have blkdev->filename = NULL. Doesn't this
>> just move the crash a few lines downwards when bdrv_open() tries to use
>> NULL as its filename?
>
> There is a check on blkdev->params being NULL few lines after so we just
> return.
Thanks, I missed that one.
> Maybe an explicit return -1 like in the appended patch here would be
> better?
> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> index 134ac33..fc0de14 100644
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -582,6 +582,9 @@ static int blk_init(struct XenDevice *xendev)
> /* read xenstore entries */
> if (blkdev->params == NULL) {
> blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
> + if (blkdev->params == NULL) {
> + return -1;
> + }
> h = strchr(blkdev->params, ':');
> if (h != NULL) {
> blkdev->fileproto = blkdev->params;
Yes, I think this is more explicit, and therefore easier to read.
Kevin