linux-kernel - Re: [RFC PATCH 33/33] Add Xen virtual block device driver.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 18 Jul 2006 12:28:28 -0700
From:	Chris Wright <chrisw@...s-sol.org>
To:	Dave Boutcher <boutcher@...umn.edu>
Cc:	Chris Wright <chrisw@...s-sol.org>, linux-kernel@...r.kernel.org,
	virtualization@...ts.osdl.org, xen-devel@...ts.xensource.com,
	Jeremy Fitzhardinge <jeremy@...p.org>, Andi Kleen <ak@...e.de>,
	Andrew Morton <akpm@...l.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Zachary Amsden <zach@...are.com>,
	Ian Pratt <ian.pratt@...source.com>,
	Christian Limpach <Christian.Limpach@...cam.ac.uk>
Subject: Re: [RFC PATCH 33/33] Add Xen virtual block device driver.

* Dave Boutcher (boutcher@...umn.edu) wrote:
> On Tue, 18 Jul 2006 00:00:33 -0700, Chris Wright <chrisw@...s-sol.org> said:
> > 
> > The block device frontend driver allows the kernel to access block
> > devices exported exported by a virtual machine containing a physical
> > block device driver.
> 
> First, I think this belongs in drivers/block (and the network driver
> belongs in drivers/net).  If we're going to bring xen to the party,
> lets not leave it hiding out in a corner.

Yeah, I think so too.

> > +	switch (backend_state) {
> > +	case XenbusStateUnknown:
> > +	case XenbusStateInitialising:
> > +	case XenbusStateInitWait:
> > +	case XenbusStateInitialised:
> > +	case XenbusStateClosed:
> 
> This actually should get fixed elsewhere, but SillyCaps???

I really don't care either way.  There's no shortage of this style
specifically for enums, in fact there's a wide variety of interesting
styles for enums.

> > +static inline int GET_ID_FROM_FREELIST(
> > +	struct blkfront_info *info)
> > +{
> > +	unsigned long free = info->shadow_free;
> > +	BUG_ON(free > BLK_RING_SIZE);
> > +	info->shadow_free = info->shadow[free].req.id;
> > +	info->shadow[free].req.id = 0x0fffffee; /* debug */
> > +	return free;
> > +}
> > +
> > +static inline void ADD_ID_TO_FREELIST(
> > +	struct blkfront_info *info, unsigned long id)
> > +{
> > +	info->shadow[id].req.id  = info->shadow_free;
> > +	info->shadow[id].request = 0;
> > +	info->shadow_free = id;
> > +}
> 
> A real nit..but why are these routines SHOUTING?

GOOD QUESTION!  I had missed that, thanks.  Seems likely it was just half
converted from macro. I see no reason not to clean that up the rest of
the way.

> > +int blkif_release(struct inode *inode, struct file *filep)
> > +{
> > +	struct blkfront_info *info = inode->i_bdev->bd_disk->private_data;
> > +	info->users--;
> > +	if (info->users == 0) {
> 
> Hrm...this strikes me as racey.  Don't you need at least a memory
> barrier here to handle SMP?
> 
> > +static struct xlbd_major_info xvd_major_info = {
> > +	.major = 201,
> > +	.type = &xvd_type_info
> > +};
> 
> I've forgotten what the current policy is around new major numbers. 

Damn, this is rather funny.  There was a number reserved, but somehow
this is the wrong one (should be 202 according to lanana[1]).  Thanks,
I'll fix that up.

thanks,
-chris

[1] http://www.lanana.org/docs/device-list/devices-2.6+.txt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/