| Message ID | 96FCDECA-CC6E-4E30-A812-40FA3CA78C31@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 28 December 2014 at 21:18, Programmingkid <programmingkidx@gmail.com> wrote: > This patch fixes the problem with raw_getlength() on Mac OS X so that it actually calculates the correct size of a volume. It has been updated to fix certain coding style issues. Booting and using a real CD in QEMU works again. > > signed-off-by: John Arbuckle <programmingkidx@gmail.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> -- PMM
On Sun, Dec 28, 2014 at 04:18:38PM -0500, Programmingkid wrote: Suggestion for concise subject line: block/raw-posix: fix raw_getlength() for host CD-ROMs on Mac > This patch fixes the problem with raw_getlength() on Mac OS X so that it actually calculates the correct size of a volume. It has been updated to fix certain coding style issues. Booting and using a real CD in QEMU works again. Plain text emails are usually wrapped at 72 characters. It makes it easier to read the git log if you wrap lines. Good job braving the nasty nest of #ifdefs in raw_getlength() :). The code change looks good. Minor changes below - normally I'd make them while merging your patch but I don't compile QEMU on Mac so I can't compile test it. Please send a new version of this patch. > > signed-off-by: John Arbuckle <programmingkidx@gmail.com> Not sure if tags are case-sensistive but it is usually written as "Signed-off-by:". The git -s option does this for you, that's usually more convenient than typing it out manually. > diff --git a/block/raw-posix.c b/block/raw-posix.c > index e51293a..a090c9c 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1312,7 +1312,24 @@ again: > if (size == 0) > #endif > #if defined(__APPLE__) && defined(__MACH__) > - size = LLONG_MAX; > + { > + uint64_t sectors = 0; > + uint32_t sectorSize = 0; Please follow QEMU coding style and use lower_case_with_underscore variable names. > + int ret; No need to shadow the local variable that was already declared at the top of the function. Please drop this.
On Jan 2, 2015, at 9:38 AM, Stefan Hajnoczi wrote: > On Sun, Dec 28, 2014 at 04:18:38PM -0500, Programmingkid wrote: > > Suggestion for concise subject line: > > block/raw-posix: fix raw_getlength() for host CD-ROMs on Mac > >> This patch fixes the problem with raw_getlength() on Mac OS X so that it actually calculates the correct size of a volume. It has been updated to fix certain coding style issues. Booting and using a real CD in QEMU works again. > > Plain text emails are usually wrapped at 72 characters. It makes it > easier to read the git log if you wrap lines. On my email program, the lines just wrap at the width of the window they are in. Maybe there is a feature in your email program to wrap lines. > > Good job braving the nasty nest of #ifdefs in raw_getlength() :). The > code change looks good. Minor changes below - normally I'd make them > while merging your patch but I don't compile QEMU on Mac so I can't > compile test it. Please send a new version of this patch. Thank you for the kind remarks. I can't believe some of the code in that file was actually committed. I think someone should remove most of the #ifdefs and pretty up the file. I would probably do something like mac_raw_getlength(), windows_raw_getlength(), linux_raw_getlength(), sun_raw_getlength()... Then just use the correct function for the host. > >> >> signed-off-by: John Arbuckle <programmingkidx@gmail.com> > > Not sure if tags are case-sensistive but it is usually written as > "Signed-off-by:". The git -s option does this for you, that's usually > more convenient than typing it out manually. Ok, that will be easy to fix. > >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index e51293a..a090c9c 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -1312,7 +1312,24 @@ again: >> if (size == 0) >> #endif >> #if defined(__APPLE__) && defined(__MACH__) >> - size = LLONG_MAX; >> + { >> + uint64_t sectors = 0; >> + uint32_t sectorSize = 0; > > Please follow QEMU coding style and use lower_case_with_underscore > variable names. Fixed. > >> + int ret; > > No need to shadow the local variable that was already declared at the > top of the function. Please drop this. Done. Thank you for reviewing my patch.
On 2 January 2015 at 16:47, Programmingkid <programmingkidx@gmail.com> wrote: > On Jan 2, 2015, at 9:38 AM, Stefan Hajnoczi wrote: >> Plain text emails are usually wrapped at 72 characters. It makes it >> easier to read the git log if you wrap lines. > > On my email program, the lines just wrap at the width of the window they are in. > Maybe there is a feature in your email program to wrap lines. The standard git tools for applying patches from email don't wrap lines -- they assume that the sender has inserted manual carriage returns at the right places (and that if there are long lines it's because they should be long, for instance to avoid quoted command lines being wrapped). This is just one of those things where you need to follow the conventions to make life easier for the recipients, I'm afraid. >> Good job braving the nasty nest of #ifdefs in raw_getlength() :). The >> code change looks good. Minor changes below - normally I'd make them >> while merging your patch but I don't compile QEMU on Mac so I can't >> compile test it. Please send a new version of this patch. > > Thank you for the kind remarks. I can't believe some of the code in that file was > actually committed. I think someone should remove most of the #ifdefs and > pretty up the file. I would probably do something like mac_raw_getlength(), > windows_raw_getlength(), linux_raw_getlength(), sun_raw_getlength()... > Then just use the correct function for the host. Yes; you can see already that half of the BSDs have been abstracted out. The difficulty is that to a first approximation none of the regular developers here uses anything except Linux, and it's always a bit tricky to make changes which you aren't testing... thanks -- PMM
diff --git a/block/raw-posix.c b/block/raw-posix.c index e51293a..a090c9c 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1312,7 +1312,24 @@ again: if (size == 0) #endif #if defined(__APPLE__) && defined(__MACH__) - size = LLONG_MAX; + { + uint64_t sectors = 0; + uint32_t sectorSize = 0; + int ret; + + /* Query the number of sectors on the disk */ + ret = ioctl(fd, DKIOCGETBLOCKCOUNT, §ors); + if (ret == -1) { + return -errno; + } + + /* Query the size of each sector */ + ret = ioctl(fd, DKIOCGETBLOCKSIZE, §orSize); + if (ret == -1) { + return -errno; + } + size = sectors * sectorSize; + } #else size = lseek(fd, 0LL, SEEK_END); if (size < 0) {
This patch fixes the problem with raw_getlength() on Mac OS X so that it actually calculates the correct size of a volume. It has been updated to fix certain coding style issues. Booting and using a real CD in QEMU works again. signed-off-by: John Arbuckle <programmingkidx@gmail.com> --- block/raw-posix.c | 19 ++++++++++++++++++- 1 files changed, 18 insertions(+), 1 deletions(-)