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 <stefano.stabell...@eu.citrix.com> >>> >>> >>> 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