From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH 3/3] Inform users about busy device assignment attempt
Date: Tue, 15 Dec 2009 23:09:06 +0200
Message-ID: <20091215210906.GC26712@redhat.com>
References: <1260901828-5145-1-git-send-email-agraf@suse.de> <1260901828-5145-4-git-send-email-agraf@suse.de> <20091215202852.GH15084@x200.localdomain>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Alexander Graf <agraf@suse.de>, kvm list <kvm@vger.kernel.org>,
	Gleb Natapov <gleb@redhat.com>,
	Muli Ben-Yehuda <muli@il.ibm.com>,
	"Daniel P. Berrange" <berrange@redhat.com>
To: Chris Wright <chrisw@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:42653 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1760221AbZLOVLy (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 15 Dec 2009 16:11:54 -0500
Content-Disposition: inline
In-Reply-To: <20091215202852.GH15084@x200.localdomain>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Tue, Dec 15, 2009 at 12:28:52PM -0800, Chris Wright wrote:
> * Alexander Graf (agraf@suse.de) wrote:
> > +static void assign_failed_examine(AssignedDevice *dev)
> > +{
> > +    char name[PATH_MAX], dir[PATH_MAX], driver[PATH_MAX] = {}, *ns;
> > +    uint16_t vendor_id, device_id;
> > +    int r;
> > +
> > +    /* XXX implement multidomain */
> > +    sprintf(dir, "/sys/bus/pci/devices/0000:%02x:%02x.%01x/",
> > +             dev->host.bus, dev->host.dev, dev->host.func);
> > +
> > +    sprintf(name, "%sdriver", dir);
> > +
> > +    r = readlink(name, driver, sizeof(driver));
> > +    if ((r <= 0) || !(ns = strrchr(driver, '/')) || r >= sizeof(driver)) {
> 
> While the symlink should never be that long, I think you want to
> check bytes stored in driver before strrchr, else you may walk off a
> non-NULL-terminated buffer.

On I missed this.  Yes, sizeof check must be done before strrchr.
Good catch!

> > +        goto fail;
> > +    }
> > +
> > +    ns++;
> > +
> > +    if (get_real_vendor_id(dir, &vendor_id) ||
> > +        get_real_device_id(dir, &device_id)) {
> > +        goto fail;
> > +    }
> > +
> > +    fprintf(stderr, "*** The driver '%s' is occupying your device "
> > +                    "%02x:%02x.%x.\n",
> > +            ns, dev->host.bus, dev->host.dev, dev->host.func);
> > +    fprintf(stderr, "***\n");
> > +    fprintf(stderr, "*** You can try the following commands to free it:\n");
> > +    fprintf(stderr, "***\n");
> > +    fprintf(stderr, "*** $ echo \"%04x %04x\" > /sys/bus/pci/drivers/pci-stub/"
> > +                    "new_id\n", vendor_id, device_id);
> > +    fprintf(stderr, "*** $ echo \"0000:%02x:%02x.%x\" > /sys/bus/pci/drivers/"
> > +                    "%s/unbind\n",
> > +            dev->host.bus, dev->host.dev, dev->host.func, ns);
> > +    fprintf(stderr, "*** $ echo \"0000:%02x:%02x.%x\" > /sys/bus/pci/drivers/"
> > +                    "pci-stub/bind\n",
> > +            dev->host.bus, dev->host.dev, dev->host.func);
> > +    fprintf(stderr, "*** $ echo \"%04x %04x\" > /sys/bus/pci/drivers/pci-stub"
> > +                    "/remove_id\n", vendor_id, device_id);
> > +    fprintf(stderr, "***\n");
> > +
> > +    return;
> > +
> > +fail:
> > +    fprintf(stderr, "Couldn't find out why.\n");
> > +}
> > +
> >  static int assign_device(AssignedDevice *dev)
> >  {
> >      struct kvm_assigned_pci_dev assigned_dev_data;
> > @@ -781,9 +829,12 @@ static int assign_device(AssignedDevice *dev)
> >  #endif
> >  
> >      r = kvm_assign_pci_device(kvm_context, &assigned_dev_data);
> > -    if (r < 0)
> > -	fprintf(stderr, "Failed to assign device \"%s\" : %s\n",
> > +    if (r < 0) {
> > +        fprintf(stderr, "Failed to assign device \"%s\" : %s\n",
> >                  dev->dev.qdev.id, strerror(-r));
> > +
> > +        assign_failed_examine(dev);
> 
> Only really catching the EBUSY case, maybe test for explicitly?