| Message ID | 1455904915-45056-1-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
El 19/2/16 a les 19:01, Roger Pau Monne ha escrit: > This was introduced by 97ee1f (~5 years ago), but was probably never > surfaced because most people used regular files as CDROM images, so the PHY > backend was actually never selected. A year ago this was changed, and now > regular RAW files are also handled by the PHY backend, which has made this > bug suface. > > Fix it by allowing empty disks to use the PHY backend, skipping the stat > tests. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Reported-by: Alex Braunegg <alex.braunegg@gmail.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Alex Braunegg <alex.braunegg@gmail.com> > --- > Changes since v6: > - Turn the assert into a check at libxl__device_disk_set_backend. > > Changes since v4: > - Split form the rest of the series. > - Fix disk_try_backend. Ping?
Roger Pau Monne writes ("[PATCH v7] libxl: allow 'phy' backend to use empty files"):
> This was introduced by 97ee1f (~5 years ago), but was probably never
> surfaced because most people used regular files as CDROM images, so the PHY
> backend was actually never selected. A year ago this was changed, and now
> regular RAW files are also handled by the PHY backend, which has made this
> bug suface.
>
> Fix it by allowing empty disks to use the PHY backend, skipping the stat
> tests.
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 8bb5e93..e0a81e3 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -196,6 +196,12 @@ static int disk_try_backend(disk_try_backend_args *a,
> goto bad_format;
> }
>
> + if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY) {
> + LOG(DEBUG, "Disk vdev=%s is empty, skipping physical device check",
> + a->disk->vdev);
> + return backend;
This implicitly assumes that every backend can cope with absent
devices. I don't think that is necessarily true. I think this check
should be in what is now libxl__try_phy_backend.
And skipping the other tests is probably not right either. For
example, checking the backend_domid is probably still necessary (and
maybe the script too). For LIBXL_DISK_BACKEND_TAP, we still need to
check libxl__blktap_enabled. etc.
> @@ -273,6 +279,12 @@ int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) {
> LOG(ERROR, "Disk vdev=%s is empty but not cdrom", disk->vdev);
> return ERROR_INVAL;
> }
> + if (disk->pdev_path != NULL && strcmp(disk->pdev_path, "")) {
> + LOG(ERROR,
> + "Disk vdev=%s is empty but an image has been provided: %s",
> + disk->vdev, disk->pdev_path);
> + return ERROR_INVAL;
> + }
This hunk is fine.
Thanks,
Ian.
On Thu, 3 Mar 2016, Ian Jackson wrote: > Roger Pau Monne writes ("[PATCH v7] libxl: allow 'phy' backend to use empty files"): > > This was introduced by 97ee1f (~5 years ago), but was probably never > > surfaced because most people used regular files as CDROM images, so the PHY > > backend was actually never selected. A year ago this was changed, and now > > regular RAW files are also handled by the PHY backend, which has made this > > bug suface. > > > > Fix it by allowing empty disks to use the PHY backend, skipping the stat > > tests. > > > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c > > index 8bb5e93..e0a81e3 100644 > > --- a/tools/libxl/libxl_device.c > > +++ b/tools/libxl/libxl_device.c > > @@ -196,6 +196,12 @@ static int disk_try_backend(disk_try_backend_args *a, > > goto bad_format; > > } > > > > + if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY) { > > + LOG(DEBUG, "Disk vdev=%s is empty, skipping physical device check", > > + a->disk->vdev); > > + return backend; > > This implicitly assumes that every backend can cope with absent > devices. I don't think that is necessarily true. I think this check > should be in what is now libxl__try_phy_backend. This check is done inside of a switch branch that's only used by LIBXL_DISK_BACKEND_PHY, so it doesn't assume that every backend can handle empty files. > And skipping the other tests is probably not right either. For > example, checking the backend_domid is probably still necessary (and > maybe the script too). For LIBXL_DISK_BACKEND_TAP, we still need to > check libxl__blktap_enabled. etc. I can move the check to a lower place (after the other checks), but those are not error checking, they are just checks to make sure the right backend is used, just like this one. I don't see how altering their order is going to make any difference. Roger.
Roger Pau Monné writes ("Re: [PATCH v7] libxl: allow 'phy' backend to use empty files"):
> This check is done inside of a switch branch that's only used by
> LIBXL_DISK_BACKEND_PHY, so it doesn't assume that every backend can handle
> empty files.
...
> I can move the check to a lower place (after the other checks), but those
> are not error checking, they are just checks to make sure the right
> backend is used, just like this one. I don't see how altering their order
> is going to make any difference.
Thanks for your replies, which convinced me. I have rebased your
patch onto staging, fixing the trivial conflict with the colo series,
and queued it for my next push.
Ian.
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c index 8bb5e93..e0a81e3 100644 --- a/tools/libxl/libxl_device.c +++ b/tools/libxl/libxl_device.c @@ -196,6 +196,12 @@ static int disk_try_backend(disk_try_backend_args *a, goto bad_format; } + if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY) { + LOG(DEBUG, "Disk vdev=%s is empty, skipping physical device check", + a->disk->vdev); + return backend; + } + if (a->disk->backend_domid != LIBXL_TOOLSTACK_DOMID) { LOG(DEBUG, "Disk vdev=%s, is using a storage driver domain, " "skipping physical device check", a->disk->vdev); @@ -273,6 +279,12 @@ int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) { LOG(ERROR, "Disk vdev=%s is empty but not cdrom", disk->vdev); return ERROR_INVAL; } + if (disk->pdev_path != NULL && strcmp(disk->pdev_path, "")) { + LOG(ERROR, + "Disk vdev=%s is empty but an image has been provided: %s", + disk->vdev, disk->pdev_path); + return ERROR_INVAL; + } memset(&a.stab, 0, sizeof(a.stab)); } else if ((disk->backend == LIBXL_DISK_BACKEND_UNKNOWN || disk->backend == LIBXL_DISK_BACKEND_PHY) &&
This was introduced by 97ee1f (~5 years ago), but was probably never surfaced because most people used regular files as CDROM images, so the PHY backend was actually never selected. A year ago this was changed, and now regular RAW files are also handled by the PHY backend, which has made this bug suface. Fix it by allowing empty disks to use the PHY backend, skipping the stat tests. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reported-by: Alex Braunegg <alex.braunegg@gmail.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Ian Campbell <ian.campbell@citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Alex Braunegg <alex.braunegg@gmail.com> --- Changes since v6: - Turn the assert into a check at libxl__device_disk_set_backend. Changes since v4: - Split form the rest of the series. - Fix disk_try_backend. --- tools/libxl/libxl_device.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)